[InstCombine] Remove AllOnes fallbacks in getMaskedTypeForICmpPair()

[nikic]

getMaskedTypeForICmpPair() tries to model non-and operands as x & -1. However, this can end up confusing the matching logic, by picking the -1 operand as the "common" operand, resulting in a successful, but useless, match. This is what causes commutation failures for some of the optimizations driven by this function.

Fix this by treating a match against -1 as a non-match.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Nikita Popov (nikic)

Changes

getMaskedTypeForICmpPair() tries to model non-and operands as x & -1. However, this can end up confusing the matching logic, by picking the -1 operand as the "common" operand, resulting in a successful, but useless, match. This is what causes commutation failures for some of the optimizations driven by this function.

Fix this by removing this -1 fallback entirely. We don't seem to have any test coverage that demonstrates why it would be needed.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp (+33-50)
• (modified) llvm/test/Transforms/InstCombine/bit-checks.ll (+8-12)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index 2bba83b5cde3c7..e7f21a42105add 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -211,23 +211,18 @@ static std::optional<std::pair<unsigned, unsigned>> getMaskedTypeForICmpPair(
   // above.
   Value *L1 = LHS->getOperand(0);
   Value *L2 = LHS->getOperand(1);
-  Value *L11, *L12, *L21, *L22;
+  Value *L11 = nullptr, *L12 = nullptr, *L21 = nullptr, *L22 = nullptr;
   // Check whether the icmp can be decomposed into a bit test.
   if (decomposeBitTestICmp(L1, L2, PredL, L11, L12, L2)) {
     L21 = L22 = L1 = nullptr;
   } else {
     // Look for ANDs in the LHS icmp.
-    if (!match(L1, m_And(m_Value(L11), m_Value(L12)))) {
-      // Any icmp can be viewed as being trivially masked; if it allows us to
-      // remove one, it's worth it.
-      L11 = L1;
-      L12 = Constant::getAllOnesValue(L1->getType());
-    }
+    match(L1, m_And(m_Value(L11), m_Value(L12)));
+    match(L2, m_And(m_Value(L21), m_Value(L22)));
 
-    if (!match(L2, m_And(m_Value(L21), m_Value(L22)))) {
-      L21 = L2;
-      L22 = Constant::getAllOnesValue(L2->getType());
-    }
+    // Check that at least one and was found.
+    if (!L11 && !L21)
+      return std::nullopt;
   }
 
   // Bail if LHS was a icmp that can't be decomposed into an equality.
@@ -252,54 +247,42 @@ static std::optional<std::pair<unsigned, unsigned>> getMaskedTypeForICmpPair(
     R1 = nullptr;
     Ok = true;
   } else {
-    if (!match(R1, m_And(m_Value(R11), m_Value(R12)))) {
-      // As before, model no mask as a trivial mask if it'll let us do an
-      // optimization.
-      R11 = R1;
-      R12 = Constant::getAllOnesValue(R1->getType());
+    if (match(R1, m_And(m_Value(R11), m_Value(R12)))) {
+      if (R11 == L11 || R11 == L12 || R11 == L21 || R11 == L22) {
+        A = R11;
+        D = R12;
+        E = R2;
+        Ok = true;
+      } else if (R12 == L11 || R12 == L12 || R12 == L21 || R12 == L22) {
+        A = R12;
+        D = R11;
+        E = R2;
+        Ok = true;
+      }
     }
 
-    if (R11 == L11 || R11 == L12 || R11 == L21 || R11 == L22) {
-      A = R11;
-      D = R12;
-      E = R2;
-      Ok = true;
-    } else if (R12 == L11 || R12 == L12 || R12 == L21 || R12 == L22) {
-      A = R12;
-      D = R11;
-      E = R2;
-      Ok = true;
+    if (match(R2, m_And(m_Value(R11), m_Value(R12)))) {
+      if (R11 == L11 || R11 == L12 || R11 == L21 || R11 == L22) {
+        A = R11;
+        D = R12;
+        E = R1;
+        Ok = true;
+      } else if (R12 == L11 || R12 == L12 || R12 == L21 || R12 == L22) {
+        A = R12;
+        D = R11;
+        E = R1;
+        Ok = true;
+      }
     }
+
+    if (!Ok)
+      return std::nullopt;
   }
 
   // Bail if RHS was a icmp that can't be decomposed into an equality.
   if (!ICmpInst::isEquality(PredR))
     return std::nullopt;
 
-  // Look for ANDs on the right side of the RHS icmp.
-  if (!Ok) {
-    if (!match(R2, m_And(m_Value(R11), m_Value(R12)))) {
-      R11 = R2;
-      R12 = Constant::getAllOnesValue(R2->getType());
-    }
-
-    if (R11 == L11 || R11 == L12 || R11 == L21 || R11 == L22) {
-      A = R11;
-      D = R12;
-      E = R1;
-      Ok = true;
-    } else if (R12 == L11 || R12 == L12 || R12 == L21 || R12 == L22) {
-      A = R12;
-      D = R11;
-      E = R1;
-      Ok = true;
-    } else {
-      return std::nullopt;
-    }
-
-    assert(Ok && "Failed to find AND on the right side of the RHS icmp.");
-  }
-
   if (L11 == A) {
     B = L12;
     C = L2;
diff --git a/llvm/test/Transforms/InstCombine/bit-checks.ll b/llvm/test/Transforms/InstCombine/bit-checks.ll
index c7e1fbb8945493..906e57e0979635 100644
--- a/llvm/test/Transforms/InstCombine/bit-checks.ll
+++ b/llvm/test/Transforms/InstCombine/bit-checks.ll
@@ -809,12 +809,10 @@ define i32 @main7a_logical(i32 %argc, i32 %argc2, i32 %argc3) {
 define i32 @main7b(i32 %argc, i32 %argc2, i32 %argc3x) {
 ; CHECK-LABEL: @main7b(
 ; CHECK-NEXT:    [[ARGC3:%.*]] = mul i32 [[ARGC3X:%.*]], 42
-; CHECK-NEXT:    [[AND1:%.*]] = and i32 [[ARGC:%.*]], [[ARGC2:%.*]]
-; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp ne i32 [[AND1]], [[ARGC2]]
-; CHECK-NEXT:    [[AND2:%.*]] = and i32 [[ARGC3]], [[ARGC]]
-; CHECK-NEXT:    [[TOBOOL3:%.*]] = icmp ne i32 [[ARGC3]], [[AND2]]
-; CHECK-NEXT:    [[AND_COND_NOT:%.*]] = or i1 [[TOBOOL]], [[TOBOOL3]]
-; CHECK-NEXT:    [[STOREMERGE:%.*]] = zext i1 [[AND_COND_NOT]] to i32
+; CHECK-NEXT:    [[TMP1:%.*]] = or i32 [[ARGC3]], [[ARGC2:%.*]]
+; CHECK-NEXT:    [[TMP2:%.*]] = and i32 [[TMP1]], [[ARGC:%.*]]
+; CHECK-NEXT:    [[AND_COND:%.*]] = icmp ne i32 [[TMP2]], [[TMP1]]
+; CHECK-NEXT:    [[STOREMERGE:%.*]] = zext i1 [[AND_COND]] to i32
 ; CHECK-NEXT:    ret i32 [[STOREMERGE]]
 ;
   %argc3 = mul i32 %argc3x, 42 ; thwart complexity-based canonicalization
@@ -850,12 +848,10 @@ define i32 @main7b_logical(i32 %argc, i32 %argc2, i32 %argc3) {
 define i32 @main7c(i32 %argc, i32 %argc2, i32 %argc3x) {
 ; CHECK-LABEL: @main7c(
 ; CHECK-NEXT:    [[ARGC3:%.*]] = mul i32 [[ARGC3X:%.*]], 42
-; CHECK-NEXT:    [[AND1:%.*]] = and i32 [[ARGC2:%.*]], [[ARGC:%.*]]
-; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp ne i32 [[AND1]], [[ARGC2]]
-; CHECK-NEXT:    [[AND2:%.*]] = and i32 [[ARGC3]], [[ARGC]]
-; CHECK-NEXT:    [[TOBOOL3:%.*]] = icmp ne i32 [[ARGC3]], [[AND2]]
-; CHECK-NEXT:    [[AND_COND_NOT:%.*]] = or i1 [[TOBOOL]], [[TOBOOL3]]
-; CHECK-NEXT:    [[STOREMERGE:%.*]] = zext i1 [[AND_COND_NOT]] to i32
+; CHECK-NEXT:    [[TMP1:%.*]] = or i32 [[ARGC3]], [[ARGC2:%.*]]
+; CHECK-NEXT:    [[TMP2:%.*]] = and i32 [[TMP1]], [[ARGC:%.*]]
+; CHECK-NEXT:    [[AND_COND:%.*]] = icmp ne i32 [[TMP2]], [[TMP1]]
+; CHECK-NEXT:    [[STOREMERGE:%.*]] = zext i1 [[AND_COND]] to i32
 ; CHECK-NEXT:    ret i32 [[STOREMERGE]]
 ;
   %argc3 = mul i32 %argc3x, 42 ; thwart complexity-based canonicalization

[dtcxzyw]

Regression:

define i1 @test(i32 %104, i32 %106, i32 noundef %.fr.i) {
  %206 = icmp ne i32 %104, 0
  %207 = icmp ne i32 %106, 63
  %or.cond19.i = select i1 %206, i1 true, i1 %207
  %208 = icmp ne i32 %.fr.i, 0
  %or.cond21.i = or i1 %or.cond19.i, %208
  ret i1 %or.cond21.i
}

should be folded into

define i1 @test(i32 %0, i32 %1, i32 noundef %.fr.i) {
  %3 = icmp ne i32 %1, 63
  %4 = or i32 %0, %.fr.i
  %5 = icmp ne i32 %4, 0
  %or.cond21.i = select i1 %5, i1 true, i1 %3
  ret i1 %or.cond21.i
}

[goldsteinn]

Would a more direct fix for the specific issue just be Ok &= !match(A, m_AllOnes())?

[nikic]

I went with this approach.

Annoyingly, @dtcxzyw's case does actually need to match on -1 here. I ended up handling that case separately via 32679e1.

[nikic]

Another test case:

define i1 @test2(i64 %a) {
  %cmp1 = icmp ne i64 %a, 0
  %a.mask = and i64 %a, 9223372036854775807
  %cmp2 = icmp eq i64 %a.mask, 0
  %and = and i1 %cmp1, %cmp2 
  ret i1 %and 
}

define i1 @test2(i64 %a) {
  %and = icmp eq i64 %a, -9223372036854775808
  ret i1 %and
}

[goldsteinn]

LGTM

Edit: Assuming @dtcxzyw's benchmarks don't show regression anymore.

[dtcxzyw]

LG