[PATCH] D82005: [InstCombine] Replace selects with Phis

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

In D82005#2105903 <https://reviews.llvm.org/D82005#2105903>, @mkazantsev wrote:

> > An incoming multi-edge (same predecessor occurring multiple times) from a switch.
>
> Not sure why it is important, but ok, will do.


Thanks!

>> A select operand coming from the terminator of a predecessor block, e.g. an invoke result.
> 
> We do not support that. Do you want a negative test?

What I actually had in mind here is something along these lines:

  declare i32 @__gxx_personality_v0(...)
  declare i32 @foo()
  
  define i32 @test_invoke_neg(i1 %cond, i32 %x, i32 %y) nounwind uwtable ssp personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
  ; CHECK-LABEL: @test_invoke_neg(
  ; CHECK-NEXT:  entry:
  ; CHECK-NEXT:    br i1 [[COND:%.*]], label [[IF_TRUE:%.*]], label [[IF_FALSE:%.*]]
  ; CHECK:       if.true:
  ; CHECK-NEXT:    br label [[MERGE:%.*]]
  ; CHECK:       if.false:
  ; CHECK-NEXT:    [[RESULT:%.*]] = invoke i32 @foo()
  ; CHECK-NEXT:    to label [[MERGE]] unwind label [[LPAD:%.*]]
  ; CHECK:       merge:
  ; CHECK-NEXT:    [[PHI:%.*]] = phi i32 [ 0, [[IF_TRUE]] ], [ [[RESULT]], [[IF_FALSE]] ]
  ; CHECK-NEXT:    [[SEL:%.*]] = select i1 [[COND]], i32 1, i32 [[PHI]]
  ; CHECK-NEXT:    ret i32 [[SEL]]
  ; CHECK:       lpad:
  ; CHECK-NEXT:    [[LP:%.*]] = landingpad { i1, i32 }
  ; CHECK-NEXT:    filter [0 x i1] zeroinitializer
  ; CHECK-NEXT:    unreachable
  ;
  entry:
    br i1 %cond, label %if.true, label %if.false
  
  if.true:
    br label %merge
  
  if.false:
    %result = invoke i32 @foo()
    to label %merge unwind label %lpad
  
  merge:
    %phi = phi i32 [ 0, %if.true ], [ %result, %if.false ]
    %sel = select i1 %cond, i32 1, i32 %phi
    ret i32 %sel
  
  lpad:
    %lp = landingpad { i1, i32 }
    filter [0 x i1] zeroinitializer
    unreachable
  }

That is, a case where the select input exactly dominates the terminator of the predecessor. But now that I wrote that, it's not really applicable to this patch, because there's the phi node sitting in between, so this is more a test for D82072 <https://reviews.llvm.org/D82072> than this one.

>> A select with a very obviously non-dominating input.
> 
> A select cannot have a non-dominating input.

Sorry, what I meant here is that the input does not dominate the terminator of the predecessor. You do have some tests of this kind, but only as a side-effect of instructions being sunk into the block that contains the select, while the original test cases could be transformed. The suggestion here is to add one true-negative test case that will not transform even once the possible extensions (phi translation, hoisting instructions back) to this patch are implemented. Something like:

  define i32 @test(i1 %cond, i32 %a, i32 %b) {
  ; CHECK-LABEL: @test(
  ; CHECK-NEXT:  entry:
  ; CHECK-NEXT:    br i1 [[COND:%.*]], label [[IF_TRUE:%.*]], label [[IF_FALSE:%.*]]
  ; CHECK:       if.true:
  ; CHECK-NEXT:    br label [[MERGE:%.*]]
  ; CHECK:       if.false:
  ; CHECK-NEXT:    br label [[MERGE]]
  ; CHECK:       merge:
  ; CHECK-NEXT:    [[PHI:%.*]] = phi i32 [ [[A:%.*]], [[IF_TRUE]] ], [ [[B:%.*]], [[IF_FALSE]] ]
  ; CHECK-NEXT:    [[ADD:%.*]] = add i32 [[PHI]], 1
  ; CHECK-NEXT:    [[S:%.*]] = select i1 [[COND]], i32 0, i32 [[ADD]]
  ; CHECK-NEXT:    ret i32 [[S]]
  ;
  entry:
    br i1 %cond, label %if.true, label %if.false
  
  if.true:
    br label %merge
  
  if.false:
    br label %merge
  
  merge:
    %phi = phi i32 [%a, %if.true], [%b, %if.false]
    %add = add i32 %phi, 1
    %s = select i1 %cond, i32 0, i32 %add
    ret i32 %s
  }

In any case, this LGTM.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2466
+                   m_Br(m_Not(m_Specific(Cond)), m_BasicBlock(TrueSucc),
+                        m_BasicBlock(FalseSucc)))) {
+    IfTrue = Sel.getFalseValue();
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > nikic wrote:
> > > This looks like dead code, as we canonicalize branch of not by swapping successors. Might change with D81089, but for now it should be fine to just drop it.
> > WDYM dead code? This code is needed to initialize variables `TrueSucc` and `FalseSucc`.
> By the moment when we process this Phi, we did not process its dominator block yet. So no, this branch will not be canonicalized when we are handling this one.
Hm, you're right. If I add an assert(0) in here, I do get some test-suite failures. D75362 should guarantee that the branch is canonicalized first, but until then it's not dead code indeed (though it should still picked up by fixpoint iteration, but best not rely on that).


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

https://reviews.llvm.org/D82005