[InstCombine] allow more folds for multi-use selects (2nd try)

The 1st try ( 681a6a399022 ) was reverted because it caused
a DataFlowSanitizer bot failure.

This try modifies the existing calls to simplifyBinOp() to
not use a query that sets the context instruction because
that seems like a likely source of failure. Since we already
try those simplifies with multi-use patterns in some cases,
that means the bug is likely present even without this patch.

However, I have not been able to reduce a test to prove that
this was the bug, so if we see any bot failures with this patch,
then it should be reverted again.

The reduced simplify power does not affect any optimizations
in existing, motivating regression tests.

Original commit message:

The 'and' case showed up in a recent bug report and prevented
more follow-on transforms from happening.

We could handle more patterns (for example, the select arms
simplified, but not to constant values), but this seems
like a safe, conservative enhancement. The backend can
convert select-of-constants to math/logic in many cases
if it is profitable.

There is a lot of overlapping logic for these kinds of patterns
(see SimplifySelectsFeedingBinaryOp() and FoldOpIntoSelect()),
so there may be some opportunity to improve efficiency.

There are also optimization gaps/inconsistency because we do
not call this code for all bin-opcodes (see TODO for ashr test).

diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 61bac17..58c2dec 100644 (file)
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -882,8 +882,6 @@ Value *InstCombinerImpl::SimplifySelectsFeedingBinaryOp(BinaryOperator &I,
   }
 
   Instruction::BinaryOps Opcode = I.getOpcode();
-  SimplifyQuery Q = SQ.getWithInstruction(&I);
-
   Value *Cond, *True = nullptr, *False = nullptr;
 
   // Special-case for add/negate combination. Replace the zero in the negation
@@ -910,8 +908,8 @@ Value *InstCombinerImpl::SimplifySelectsFeedingBinaryOp(BinaryOperator &I,
   if (LHSIsSelect && RHSIsSelect && A == D) {
     // (A ? B : C) op (A ? E : F) -> A ? (B op E) : (C op F)
     Cond = A;
-    True = simplifyBinOp(Opcode, B, E, FMF, Q);
-    False = simplifyBinOp(Opcode, C, F, FMF, Q);
+    True = simplifyBinOp(Opcode, B, E, FMF, SQ);
+    False = simplifyBinOp(Opcode, C, F, FMF, SQ);
 
     if (LHS->hasOneUse() && RHS->hasOneUse()) {
       if (False && !True)
@@ -919,18 +917,24 @@ Value *InstCombinerImpl::SimplifySelectsFeedingBinaryOp(BinaryOperator &I,
       else if (True && !False)
         False = Builder.CreateBinOp(Opcode, C, F);
     }
-  } else if (LHSIsSelect && LHS->hasOneUse()) {
+  } else if (LHSIsSelect) {
     // (A ? B : C) op Y -> A ? (B op Y) : (C op Y)
     Cond = A;
-    True = simplifyBinOp(Opcode, B, RHS, FMF, Q);
-    False = simplifyBinOp(Opcode, C, RHS, FMF, Q);
+    True = simplifyBinOp(Opcode, B, RHS, FMF, SQ);
+    False = simplifyBinOp(Opcode, C, RHS, FMF, SQ);
+    if (!LHS->hasOneUse() &&
+        (!True || !False || !isa<Constant>(True) || !isa<Constant>(False)))
+      return nullptr;
     if (Value *NewSel = foldAddNegate(B, C, RHS))
       return NewSel;
-  } else if (RHSIsSelect && RHS->hasOneUse()) {
+  } else if (RHSIsSelect) {
     // X op (D ? E : F) -> D ? (X op E) : (X op F)
     Cond = D;
-    True = simplifyBinOp(Opcode, LHS, E, FMF, Q);
-    False = simplifyBinOp(Opcode, LHS, F, FMF, Q);
+    True = simplifyBinOp(Opcode, LHS, E, FMF, SQ);
+    False = simplifyBinOp(Opcode, LHS, F, FMF, SQ);
+    if (!RHS->hasOneUse() &&
+        (!True || !False || !isa<Constant>(True) || !isa<Constant>(False)))
+      return nullptr;
     if (Value *NewSel = foldAddNegate(E, F, LHS))
       return NewSel;
   }

diff --git a/llvm/test/Transforms/InstCombine/binop-select.ll b/llvm/test/Transforms/InstCombine/binop-select.ll
index 90f41a2..cce1206 100644 (file)
--- a/llvm/test/Transforms/InstCombine/binop-select.ll
+++ b/llvm/test/Transforms/InstCombine/binop-select.ll
@@ -188,11 +188,13 @@ define i32 @and_sel_op0(i1 %b) {
   ret i32 %r
 }
 
+; extra use is ok
+
 define i32 @and_sel_op0_use(i1 %b) {
 ; CHECK-LABEL: @and_sel_op0_use(
 ; CHECK-NEXT:    [[S:%.*]] = select i1 [[B:%.*]], i32 25, i32 0
 ; CHECK-NEXT:    call void @use(i32 [[S]])
-; CHECK-NEXT:    [[R:%.*]] = and i32 [[S]], 1
+; CHECK-NEXT:    [[R:%.*]] = zext i1 [[B]] to i32
 ; CHECK-NEXT:    ret i32 [[R]]
 ;
   %s = select i1 %b, i32 25, i32 0
@@ -212,12 +214,14 @@ define i32 @mul_sel_op0(i1 %b, i32 %x) {
   ret i32 %r
 }
 
+; extra use is ok
+
 define i32 @mul_sel_op0_use(i1 %b, i32 %x) {
 ; CHECK-LABEL: @mul_sel_op0_use(
 ; CHECK-NEXT:    [[D:%.*]] = udiv exact i32 42, [[X:%.*]]
 ; CHECK-NEXT:    [[S:%.*]] = select i1 [[B:%.*]], i32 0, i32 [[D]]
 ; CHECK-NEXT:    call void @use(i32 [[S]])
-; CHECK-NEXT:    [[R:%.*]] = mul i32 [[S]], [[X]]
+; CHECK-NEXT:    [[R:%.*]] = select i1 [[B]], i32 0, i32 42
 ; CHECK-NEXT:    ret i32 [[R]]
 ;
   %d = udiv exact i32 42, %x
@@ -238,11 +242,14 @@ define i32 @sub_sel_op1(i1 %b) {
   ret i32 %r
 }
 
+; extra use is ok (extra instruction caused by select canonicalization to zext (!b))
+
 define i32 @sub_sel_op1_use(i1 %b) {
 ; CHECK-LABEL: @sub_sel_op1_use(
 ; CHECK-NEXT:    [[S:%.*]] = select i1 [[B:%.*]], i32 42, i32 41
 ; CHECK-NEXT:    call void @use(i32 [[S]])
-; CHECK-NEXT:    [[R:%.*]] = sub nsw i32 42, [[S]]
+; CHECK-NEXT:    [[NOT_B:%.*]] = xor i1 [[B]], true
+; CHECK-NEXT:    [[R:%.*]] = zext i1 [[NOT_B]] to i32
 ; CHECK-NEXT:    ret i32 [[R]]
 ;
   %s = select i1 %b, i32 42, i32 41
@@ -261,11 +268,13 @@ define float @fadd_sel_op0(i1 %b, float %x) {
   ret float %r
 }
 
+; extra use is ok
+
 define float @fadd_sel_op0_use(i1 %b, float %x) {
 ; CHECK-LABEL: @fadd_sel_op0_use(
 ; CHECK-NEXT:    [[S:%.*]] = select i1 [[B:%.*]], float 0xFFF0000000000000, float 0x7FF0000000000000
 ; CHECK-NEXT:    call void @use_f32(float [[S]])
-; CHECK-NEXT:    [[R:%.*]] = fadd nnan float [[S]], [[X:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = select nnan i1 [[B]], float 0xFFF0000000000000, float 0x7FF0000000000000
 ; CHECK-NEXT:    ret float [[R]]
 ;
   %s = select i1 %b, float 0xFFF0000000000000, float 0x7FF0000000000000
@@ -284,13 +293,13 @@ define <2 x half> @fmul_sel_op1(i1 %b, <2 x half> %p) {
   ret <2 x half> %r
 }
 
+; extra use is ok - poison allows simplifying
+
 define <2 x half> @fmul_sel_op1_use(i1 %b, <2 x half> %p) {
 ; CHECK-LABEL: @fmul_sel_op1_use(
-; CHECK-NEXT:    [[X:%.*]] = fadd <2 x half> [[P:%.*]], <half 0xH3C00, half 0xH4000>
 ; CHECK-NEXT:    [[S:%.*]] = select i1 [[B:%.*]], <2 x half> zeroinitializer, <2 x half> <half 0xHFFFF, half 0xHFFFF>
 ; CHECK-NEXT:    call void @use_v2f16(<2 x half> [[S]])
-; CHECK-NEXT:    [[R:%.*]] = fmul nnan nsz <2 x half> [[X]], [[S]]
-; CHECK-NEXT:    ret <2 x half> [[R]]
+; CHECK-NEXT:    ret <2 x half> zeroinitializer
 ;
   %x = fadd <2 x half> %p, <half 1.0, half 2.0> ; thwart complexity-based canonicalization
   %s = select i1 %b, <2 x half> zeroinitializer, <2 x half> <half 0xHffff, half 0xHffff>
@@ -309,6 +318,8 @@ define i32 @ashr_sel_op1(i1 %b) {
   ret i32 %r
 }
 
+; TODO: SimplifySelectsFeedingBinaryOp() is not called for shifts; previous test reduced some other way.
+
 define i32 @ashr_sel_op1_use(i1 %b) {
 ; CHECK-LABEL: @ashr_sel_op1_use(
 ; CHECK-NEXT:    [[S:%.*]] = select i1 [[B:%.*]], i32 2, i32 0