[InstCombine] Bail out on inner disjoint or in foldSelectICmpEq

[dtcxzyw]

It is tricky to remove the disjoint flag in the inner instruction. I believe these patterns with inner disjoint or are unlikely to exist in real-world cases.

Alive2: https://alive2.llvm.org/ce/z/hjyRk-
Closes #121583.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

It is tricky to remove the disjoint flag in the inner instruction. I believe these patterns with inner disjoint or are unlikely to exist in real-world cases.

Alive2: https://alive2.llvm.org/ce/z/hjyRk-
Closes #121583.

---

Full diff: https://github.com/llvm/llvm-project/pull/121635.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp (+9-11)
• (modified) llvm/test/Transforms/InstCombine/select.ll (+78)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index e7a8e947705f8d..471ffa33bdfc02 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -1850,25 +1850,23 @@ static Instruction *foldSelectICmpEq(SelectInst &SI, ICmpInst *ICI,
     return nullptr;
 
   const unsigned AndOps = Instruction::And, OrOps = Instruction::Or,
-                 XorOps = Instruction::Xor, NoOps = 0;
+                 XorOps = Instruction::Xor;
   enum NotMask { None = 0, NotInner, NotRHS };
 
+  // We cannot refine TrueVal to FalseVal when the inner instruction contains
+  // disjoint or.
   auto matchFalseVal = [&](unsigned OuterOpc, unsigned InnerOpc,
                            unsigned NotMask) {
-    auto matchInner = m_c_BinOp(InnerOpc, m_Specific(X), m_Specific(Y));
-    if (OuterOpc == NoOps)
-      return match(CmpRHS, m_Zero()) && match(FalseVal, matchInner);
-
-    if (NotMask == NotInner) {
+    auto matchInner =
+        m_CombineAnd(m_c_BinOp(InnerOpc, m_Specific(X), m_Specific(Y)),
+                     m_Unless(m_DisjointOr(m_Value(), m_Value())));
+    if (NotMask == NotInner)
       return match(FalseVal, m_c_BinOp(OuterOpc, m_NotForbidPoison(matchInner),
                                        m_Specific(CmpRHS)));
-    } else if (NotMask == NotRHS) {
+    if (NotMask == NotRHS)
       return match(FalseVal, m_c_BinOp(OuterOpc, matchInner,
                                        m_NotForbidPoison(m_Specific(CmpRHS))));
-    } else {
-      return match(FalseVal,
-                   m_c_BinOp(OuterOpc, matchInner, m_Specific(CmpRHS)));
-    }
+    return match(FalseVal, m_c_BinOp(OuterOpc, matchInner, m_Specific(CmpRHS)));
   };
 
   // (X&Y)==C ? X|Y : X^Y -> (X^Y)|C : X^Y  or (X^Y)^ C : X^Y
diff --git a/llvm/test/Transforms/InstCombine/select.ll b/llvm/test/Transforms/InstCombine/select.ll
index 0168a804239a89..2ad67c80513a2d 100644
--- a/llvm/test/Transforms/InstCombine/select.ll
+++ b/llvm/test/Transforms/InstCombine/select.ll
@@ -4564,6 +4564,84 @@ define i32 @src_no_trans_select_xor_eq0_or_xor(i32 %x, i32 %y) {
   ret i32 %cond
 }
 
+define i32 @src_no_trans_select_xor_eqc_and_disjoint_or_and_notc(i32 %x, i32 %y, i32 %c) {
+; CHECK-LABEL: @src_no_trans_select_xor_eqc_and_disjoint_or_and_notc(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[XOR]], [[C:%.*]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[X]], [[Y]]
+; CHECK-NEXT:    [[OR:%.*]] = or disjoint i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[C]], -1
+; CHECK-NEXT:    [[AND1:%.*]] = and i32 [[OR]], [[NOT]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[AND]], i32 [[AND1]]
+; CHECK-NEXT:    ret i32 [[COND]]
+;
+entry:
+  %xor = xor i32 %y, %x
+  %cmp = icmp eq i32 %xor, %c
+  %and = and i32 %x, %y
+  %or = or disjoint i32 %y, %x
+  %not = xor i32 %c, -1
+  %and1 = and i32 %or, %not
+  %cond = select i1 %cmp, i32 %and, i32 %and1
+  ret i32 %cond
+}
+
+define i32 @negative_inner_disjoint_or2(i32 %x, i32 %y, i32 %c) {
+; CHECK-LABEL: @negative_inner_disjoint_or2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[XOR]], [[C:%.*]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[X]], [[Y]]
+; CHECK-NEXT:    [[OR:%.*]] = or disjoint i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[AND1:%.*]] = xor i32 [[OR]], [[C]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[AND]], i32 [[AND1]]
+; CHECK-NEXT:    ret i32 [[COND]]
+;
+entry:
+  %xor = xor i32 %y, %x
+  %cmp = icmp eq i32 %xor, %c
+  %and = and i32 %x, %y
+  %or = or disjoint i32 %y, %x
+  %and1 = xor i32 %or, %c
+  %cond = select i1 %cmp, i32 %and, i32 %and1
+  ret i32 %cond
+}
+
+define i32 @positive_outer_disjoint_or1(i32 %x, i32 %y, i32 %c) {
+; CHECK-LABEL: @positive_outer_disjoint_or1(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[AND:%.*]] = xor i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[OR1:%.*]] = or disjoint i32 [[AND]], [[C:%.*]]
+; CHECK-NEXT:    ret i32 [[OR1]]
+;
+entry:
+  %xor = and i32 %y, %x
+  %cmp = icmp eq i32 %xor, %c
+  %or = or i32 %y, %x
+  %and = xor i32 %y, %x
+  %or1 = or disjoint i32 %and, %c
+  %cond = select i1 %cmp, i32 %or, i32 %or1
+  ret i32 %cond
+}
+
+define i32 @positive_outer_disjoint_or2(i32 %x, i32 %y, i32 %c) {
+; CHECK-LABEL: @positive_outer_disjoint_or2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[OR1:%.*]] = or disjoint i32 [[AND]], [[C:%.*]]
+; CHECK-NEXT:    ret i32 [[OR1]]
+;
+entry:
+  %xor = xor i32 %y, %x
+  %cmp = icmp eq i32 %xor, %c
+  %or = or i32 %y, %x
+  %and = and i32 %y, %x
+  %or1 = or disjoint i32 %and, %c
+  %cond = select i1 %cmp, i32 %or, i32 %or1
+  ret i32 %cond
+}
+
 ; (X == C) ? X : Y -> (X == C) ? C : Y
 ; Fixed #77553
 define i32 @src_select_xxory_eq0_xorxy_y(i32 %x, i32 %y) {

[nikic]

LGTM -- but I think we should consider deleting this code entirely. This seems to match some kind of very complicated pattern without any evidence of real-world usefulness (including no hits on llvm-opt-benchmark) and without actually handling the cases in the issue it purports to fix (#71792). It seems like through the review of #73362 this somehow moved to matching patterns that also involve an operation with C, while the original motivation only involved comparisons with 0 (where ofc 0 is not involved in the select expression). So it seems like we now ended up with overly complicated code that nobody even asked for.

[goldsteinn]

LGTM -- but I think we should consider deleting this code entirely. This seems to match some kind of very complicated pattern without any evidence of real-world usefulness (including no hits on llvm-opt-benchmark) and without actually handling the cases in the issue it purports to fix (#71792). It seems like through the review of #73362 this somehow moved to matching patterns that also involve an operation with C, while the original motivation only involved comparisons with 0 (where ofc 0 is not involved in the select expression). So it seems like we now ended up with overly complicated code that nobody even asked for.

Re the C, I think thats just misleading comments, only C == 0 is handled.

[nikic]

Are you sure? Just looking at the tests, everything that folds seems to involve a %c value and the == 0 tests are marked as TODOs.

[goldsteinn]

Are you sure? Just looking at the tests, everything that folds seems to involve a %c value and the == 0 tests are marked as TODOs.

err you're right, I was scanning for constants (although see now that code was dead!).

[goldsteinn]

LGTM -- but I think we should consider deleting this code entirely. This seems to match some kind of very complicated pattern without any evidence of real-world usefulness (including no hits on llvm-opt-benchmark) and without actually handling the cases in the issue it purports to fix (#71792). It seems like through the review of #73362 this somehow moved to matching patterns that also involve an operation with C, while the original motivation only involved comparisons with 0 (where ofc 0 is not involved in the select expression). So it seems like we now ended up with overly complicated code that nobody even asked for.

I will make a PR to replace the current impl with one that handles the C == 0 case.

[dtcxzyw]

Closed in favor of #122098