[PATCH] D125398: [ControlHeightReduction] Freeze condition when converting select to branch

Fri May 13 03:33:09 PDT 2022

nikic added inline comments.

================
Comment at: llvm/test/Transforms/PGOProfile/chr.ll:1293
+; CHECK-NEXT:    [[TMP0:%.*]] = or i1 [[V3]], [[V1_NOT]]
+; CHECK-NEXT:    br i1 [[TMP0]], label [[ENTRY_SPLIT_NONCHR:%.*]], label [[BB1:%.*]], !prof [[PROF19:![0-9]+]]
 ; CHECK:       entry.split.nonchr:
----------------
nikic wrote:
> spatel wrote:
> > nikic wrote:
> > > fhahn wrote:
> > > > nikic wrote:
> > > > > This is the only non-trivial change. Apparently it restores the result to what it was before https://github.com/llvm/llvm-project/commit/1bf8f9e228546bd54ef9739aa808b71b97ea6051, so this is probably fine.
> > > > It would be interesting to know what simplification we are missing and why. Looks like @spatel landed 1bf8f9e228546bd54ef9739aa808b71b97ea6051,, perhaps he has an idea?
> > > Hm, now that I look closer, my initial impression here was incorrect. We're missing this fold: https://alive2.llvm.org/ce/z/WrXbGk
> > Generalized:
> > https://alive2.llvm.org/ce/z/8wPmWx
> > 
> > I think instcombine gets that without the `and` interference, so there's some range optimization that might be generalized?
> > 
> > In any case, the diff from the earlier patch was a happy accident, so a regression here doesn't have to hold this patch up.
> I think the most general form of this is https://alive2.llvm.org/ce/z/NccNBe, that is `(a & b) | c -> c` if `b` implies `c`. We should be able to do something based on isImpliedCondition() here.
https://reviews.llvm.org/D125530 should fix this regression.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125398/new/

https://reviews.llvm.org/D125398