[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 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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139275