[PATCH] D139275: [SimplifyCFG] `FoldBranchToCommonDest()`: deal with mismatched IV's in PHI's in common successor block

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
Comment at: llvm/test/Transforms/SimplifyCFG/X86/SpeculativeExec.ll:15
+; CHECK-NEXT:    [[T3:%.*]] = add i32 [[A]], 1
+; CHECK-NEXT:    [[T4:%.*]] = select i1 [[OR_COND]], i32 [[T3]], i32 [[T4_SEL]]
 ; CHECK-NEXT:    [[T5:%.*]] = sub i32 [[T4]], 1
----------------
nikic wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > lebedev.ri wrote:
> > > > nikic wrote:
> > > > > nikic wrote:
> > > > > > Looks like this test, and many similar tests regress, where we go from two selects to three selects. And we never seem to recover from this in either the middle end or the back end: https://alive2.llvm.org/ce/z/GjCXkB
> > > > > Filed https://github.com/llvm/llvm-project/issues/59393. We should at least add an InstCombine fold, but ideally we'd avoid regressing this in SimplifyCFG.
> > > > I'll deal with the InstCombine side of this.
> > > > I agree that this is a regression, that obviously affects further speculation,
> > > > but i'm not sure how much we can avoid it during speculation.
> > > > I can take a look, but not in this patch,
> > > > so i hope this patch isn't going to be blocked on that.
> > > Added InstCombine fold (9ddff66d0c9c3e18d56e6b20aa26a2a8cdfb6d2b), after which we get:
> > > ```
> > > define i32 @test1(i32 %a, i32 %b, i32 %c) {
> > > entry:
> > >   %t1 = icmp eq i32 %b, 0
> > >   %t2 = icmp sgt i32 %c, 1
> > >   %t3 = zext i1 %t2 to i32
> > >   %t4.sel = add i32 %t3, %a
> > >   %t4 = select i1 %t1, i32 %t4.sel, i32 %b
> > >   %t5 = add i32 %t4, -1
> > >   ret i32 %t5
> > > }
> > > ```
> > > ... so one of the two LHS `select`s was redundant also.
> > > We may be able to improve this, but i really would prefer not to conflate things.
> > I'm going to make a guess that if that feedback
> > was meant to be blocking, it would have been marked as such,
> > and proceed with the change, acknowledging the possible further improvements.
> This is not blocking, but I did not review the implementation and only glanced over test diffs. The patch looks pretty complicated for what it does (in the sense of a minimal baseline implementation on top of which incremental changes may be stacked).
> This is not blocking

> but I did not review the implementation and only glanced over test diffs.

Right, i did not imply that you did.

> The patch looks pretty complicated for what it does (in the sense of a minimal baseline implementation on top of which incremental changes may be stacked).

Right, the non-test side of things could be much smaller if not for the abstractons.
I'm hoping to make use of them in other places we speculate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139275