Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[InstCombine] Modify foldSelectICmpEq to only handle more useful and simple cases. #121672

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[InstCombine] Modify foldSelectICmpEq to only handle more useful and simple cases. #121672

wants to merge 1 commit into from
+183 −159

Conversation

goldsteinn
Copy link
Contributor

The original intent of the folds (from #71792) where to handle selects
where the condition was a bitwise operator compared with zero.

During review of the original PR (#73362), however, we ended up over
generalizing at the expense of code complexity, and ironically in a
way that didn't actually fix the issue as reported.

The goal of this PR is to simplify the code and only handle the
compares with zero cases that actually show up in the real world.

New code handles three cases:

  1. X & Y == 0 implies X | Y == X ^ Y thus:

  2. X | Y == 0 implies X == Y == 0 thus for Op0 and Op1 s.t
    0 Op0 0 == 0 Op1 0 == 0:

  3. X ^ Y == 0 (X == Y) implies X | Y == X & Y:

@goldsteinn
[InstCombine] Modify foldSelectICmpEq to only handle more useful an…
ee59139 
…d simple cases.

The original intent of the folds (from llvm#71792) where to handle selects
where the condition was a bitwise operator compared with zero.

During review of the original PR (llvm#73362), however, we ended up over
generalizing at the expense of code complexity, and ironically in a
way that didn't actually fix the issue as reported.

The goal of this PR is to simplify the code and only handle the
compares with zero cases that actually show up in the real world.

New code handles three cases:

1) `X & Y == 0` implies `X | Y == X ^ Y` thus:
    - `X & Y == 0 ? X |/^/+ Y : X |/^/+ Y` -> `X |/^/+ Y` (the false arm)
    - https://alive2.llvm.org/ce/z/jjcduh

2) `X | Y == 0` implies `X == Y == 0` thus for `Op0` and `Op1` s.t
   `0 Op0 0 == 0 Op1 0 == 0`:
    - `X & Y == 0 ? X Op0 Y : X Op1 Y` -> `X Op1 Y`
    - `X & Y == 0 ? 0 : X Op1 Y` -> `X Op1 Y`
    - https://alive2.llvm.org/ce/z/RBuFQE

3) `X ^ Y == 0` (`X == Y`) implies `X | Y == X & Y`:
    - `X ^ Y == 0 ? X | Y : X & Y` -> `X & Y`
    - `X ^ Y == 0 ? X & Y : X | Y` -> `X | Y`
    - https://alive2.llvm.org/ce/z/SJskbz
@goldsteinn goldsteinn requested a review from nikic as a code owner January 5, 2025 01:20
@goldsteinn goldsteinn requested a review from dtcxzyw January 5, 2025 01:20
@goldsteinn goldsteinn mentioned this pull request Jan 5, 2025
Open
@llvmbot
Copy link
Member

llvmbot commented Jan 5, 2025

@llvm/pr-subscribers-llvm-transforms

Author: None (goldsteinn)

Changes

The original intent of the folds (from #71792) where to handle selects
where the condition was a bitwise operator compared with zero.

During review of the original PR (#73362), however, we ended up over
generalizing at the expense of code complexity, and ironically in a
way that didn't actually fix the issue as reported.

The goal of this PR is to simplify the code and only handle the
compares with zero cases that actually show up in the real world.

New code handles three cases:

  1. X & Y == 0 implies X | Y == X ^ Y thus:

  2. X | Y == 0 implies X == Y == 0 thus for Op0 and Op1 s.t
    0 Op0 0 == 0 Op1 0 == 0:

  3. X ^ Y == 0 (X == Y) implies X | Y == X & Y:

Patch is 22.88 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/121672.diff

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp (+67-80)
  • (modified) llvm/test/Transforms/InstCombine/select.ll (+116-79)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index e7a8e947705f8d..2838d45fcc7cd9 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -1821,8 +1821,12 @@ static Instruction *foldSelectWithExtremeEqCond(Value *CmpLHS, Value *CmpRHS,
   return new ICmpInst(Pred, CmpLHS, B);
 }
 
+// Fold (X Op0 Y) == 0 ? (X Op1 Y) : (X Op2 Y)
+//	-> (X Op2 Y)
+// By proving that `(X Op1 Y) == (X Op2 Y)` in the context of `(X Op0 Y) == 0`.
 static Instruction *foldSelectICmpEq(SelectInst &SI, ICmpInst *ICI,
                                      InstCombinerImpl &IC) {
+
   ICmpInst::Predicate Pred = ICI->getPredicate();
   if (!ICmpInst::isEquality(Pred))
     return nullptr;
@@ -1835,96 +1839,79 @@ static Instruction *foldSelectICmpEq(SelectInst &SI, ICmpInst *ICI,
   if (Pred == ICmpInst::ICMP_NE)
     std::swap(TrueVal, FalseVal);
 
-  if (Instruction *Res =
-          foldSelectWithExtremeEqCond(CmpLHS, CmpRHS, TrueVal, FalseVal))
-    return Res;
+  if (auto *R = foldSelectWithExtremeEqCond(CmpLHS, CmpRHS, TrueVal, FalseVal))
+    return R;
 
-  // Transform (X == C) ? X : Y -> (X == C) ? C : Y
-  // specific handling for Bitwise operation.
-  // x&y -> (x|y) ^ (x^y)  or  (x|y) & ~(x^y)
-  // x|y -> (x&y) | (x^y)  or  (x&y) ^  (x^y)
-  // x^y -> (x|y) ^ (x&y)  or  (x|y) & ~(x&y)
   Value *X, *Y;
-  if (!match(CmpLHS, m_BitwiseLogic(m_Value(X), m_Value(Y))) ||
-      !match(TrueVal, m_c_BitwiseLogic(m_Specific(X), m_Specific(Y))))
-    return nullptr;
-
-  const unsigned AndOps = Instruction::And, OrOps = Instruction::Or,
-                 XorOps = Instruction::Xor, NoOps = 0;
-  enum NotMask { None = 0, NotInner, NotRHS };
-
-  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) {
-      return match(FalseVal, m_c_BinOp(OuterOpc, m_NotForbidPoison(matchInner),
-                                       m_Specific(CmpRHS)));
-    } else 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)));
-    }
-  };
-
-  // (X&Y)==C ? X|Y : X^Y -> (X^Y)|C : X^Y  or (X^Y)^ C : X^Y
-  // (X&Y)==C ? X^Y : X|Y -> (X|Y)^C : X|Y  or (X|Y)&~C : X|Y
-  if (match(CmpLHS, m_And(m_Value(X), m_Value(Y)))) {
-    if (match(TrueVal, m_c_Or(m_Specific(X), m_Specific(Y)))) {
-      // (X&Y)==C ? X|Y : (X^Y)|C -> (X^Y)|C : (X^Y)|C -> (X^Y)|C
-      // (X&Y)==C ? X|Y : (X^Y)^C -> (X^Y)^C : (X^Y)^C -> (X^Y)^C
-      if (matchFalseVal(OrOps, XorOps, None) ||
-          matchFalseVal(XorOps, XorOps, None))
-        return IC.replaceInstUsesWith(SI, FalseVal);
-    } else if (match(TrueVal, m_c_Xor(m_Specific(X), m_Specific(Y)))) {
-      // (X&Y)==C ? X^Y : (X|Y)^ C -> (X|Y)^ C : (X|Y)^ C -> (X|Y)^ C
-      // (X&Y)==C ? X^Y : (X|Y)&~C -> (X|Y)&~C : (X|Y)&~C -> (X|Y)&~C
-      if (matchFalseVal(XorOps, OrOps, None) ||
-          matchFalseVal(AndOps, OrOps, NotRHS))
+  if (match(CmpRHS, m_Zero())) {
+    // (X & Y) == 0 ? X |/^/+ Y : X |/^/+ Y -> X |/^/+ Y (false arm)
+    // `(X & Y) == 0` implies no common bits which means:
+	// `X ^ Y == X | Y == X + Y`
+    // https://alive2.llvm.org/ce/z/jjcduh
+    if (match(CmpLHS, m_And(m_Value(X), m_Value(Y)))) {
+      auto MatchAddOrXor =
+          m_CombineOr(m_c_Add(m_Specific(X), m_Specific(Y)),
+                      m_CombineOr(m_c_Or(m_Specific(X), m_Specific(Y)),
+                                  m_c_Xor(m_Specific(X), m_Specific(Y))));
+      if (match(TrueVal, MatchAddOrXor) && match(FalseVal, MatchAddOrXor))
         return IC.replaceInstUsesWith(SI, FalseVal);
     }
-  }
 
-  // (X|Y)==C ? X&Y : X^Y -> (X^Y)^C : X^Y  or  ~(X^Y)&C : X^Y
-  // (X|Y)==C ? X^Y : X&Y -> (X&Y)^C : X&Y  or  ~(X&Y)&C : X&Y
-  if (match(CmpLHS, m_Or(m_Value(X), m_Value(Y)))) {
-    if (match(TrueVal, m_c_And(m_Specific(X), m_Specific(Y)))) {
-      // (X|Y)==C ? X&Y: (X^Y)^C -> (X^Y)^C: (X^Y)^C ->  (X^Y)^C
-      // (X|Y)==C ? X&Y:~(X^Y)&C ->~(X^Y)&C:~(X^Y)&C -> ~(X^Y)&C
-      if (matchFalseVal(XorOps, XorOps, None) ||
-          matchFalseVal(AndOps, XorOps, NotInner))
-        return IC.replaceInstUsesWith(SI, FalseVal);
-    } else if (match(TrueVal, m_c_Xor(m_Specific(X), m_Specific(Y)))) {
-      // (X|Y)==C ? X^Y : (X&Y)^C ->  (X&Y)^C : (X&Y)^C ->  (X&Y)^C
-      // (X|Y)==C ? X^Y :~(X&Y)&C -> ~(X&Y)&C :~(X&Y)&C -> ~(X&Y)&C
-      if (matchFalseVal(XorOps, AndOps, None) ||
-          matchFalseVal(AndOps, AndOps, NotInner))
-        return IC.replaceInstUsesWith(SI, FalseVal);
-    }
-  }
+    // (X | Y) == 0 ? X Op0 Y : X Op1 Y -> X Op1 Y
+    // For any `Op0` and `Op1` that are zero when `X` and `Y` are zero.
+	// https://alive2.llvm.org/ce/z/azHzBW
+    if (match(CmpLHS, m_Or(m_Value(X), m_Value(Y))) &&
+        (match(TrueVal, m_c_BinOp(m_Specific(X), m_Specific(Y))) ||
+		 // In true arm we can also accept just `0`.
+         match(TrueVal, m_Zero())) &&
+        match(FalseVal, m_c_BinOp(m_Specific(X), m_Specific(Y)))) {
+      auto IsOpcZeroWithZeros = [](Value *V) {
+        auto *I = dyn_cast<Instruction>(V);
+        if (!I)
+          return false;
+        switch (I->getOpcode()) {
+        case Instruction::And:
+        case Instruction::Or:
+        case Instruction::Xor:
+        case Instruction::Mul:
+        case Instruction::Add:
+        case Instruction::Sub:
+        case Instruction::Shl:
+        case Instruction::AShr:
+        case Instruction::LShr:
+          return true;
+        default:
+          return false;
+        }
+      };
 
-  // (X^Y)==C ? X&Y : X|Y -> (X|Y)^C : X|Y  or (X|Y)&~C : X|Y
-  // (X^Y)==C ? X|Y : X&Y -> (X&Y)|C : X&Y  or (X&Y)^ C : X&Y
-  if (match(CmpLHS, m_Xor(m_Value(X), m_Value(Y)))) {
-    if ((match(TrueVal, m_c_And(m_Specific(X), m_Specific(Y))))) {
-      // (X^Y)==C ? X&Y : (X|Y)^C -> (X|Y)^C
-      // (X^Y)==C ? X&Y : (X|Y)&~C -> (X|Y)&~C
-      if (matchFalseVal(XorOps, OrOps, None) ||
-          matchFalseVal(AndOps, OrOps, NotRHS))
-        return IC.replaceInstUsesWith(SI, FalseVal);
-    } else if (match(TrueVal, m_c_Or(m_Specific(X), m_Specific(Y)))) {
-      // (X^Y)==C ? (X|Y) : (X&Y)|C -> (X&Y)|C
-      // (X^Y)==C ? (X|Y) : (X&Y)^C -> (X&Y)^C
-      if (matchFalseVal(OrOps, AndOps, None) ||
-          matchFalseVal(XorOps, AndOps, None))
+      if ((match(TrueVal, m_Zero()) || IsOpcZeroWithZeros(TrueVal)) &&
+          IsOpcZeroWithZeros(FalseVal))
         return IC.replaceInstUsesWith(SI, FalseVal);
     }
   }
+  // (X == Y) ? X | Y : X & Y
+  // (X == Y) ? X & Y : X | Y
+  // If `X == Y` then `X == Y == X | Y == X & Y`.
+  // NB: `X == Y` is canonicalization of `(X ^ Y) == 0`.
+  // https://alive2.llvm.org/ce/z/SJskbz
+  X = CmpLHS;
+  Y = CmpRHS;
+  auto MatchOrAnd = m_CombineOr(m_c_Or(m_Specific(X), m_Specific(Y)),
+                                m_c_And(m_Specific(X), m_Specific(Y)));
+  if (match(FalseVal, MatchOrAnd) &&
+      // In the true arm we can also just match `X` or `Y`.
+      (match(TrueVal, MatchOrAnd) || match(TrueVal, m_Specific(X)) ||
+       match(TrueVal, m_Specific(Y)))) {
+    // Can't preserve `or disjoint` here so rebuild.
+    auto *BO = dyn_cast<BinaryOperator>(FalseVal);
+    if (!BO)
+      return nullptr;
 
+    return IC.replaceInstUsesWith(
+        SI, IC.Builder.CreateBinOp(BO->getOpcode(), BO->getOperand(0),
+                                   BO->getOperand(1)));
+  }
   return nullptr;
 }
 
diff --git a/llvm/test/Transforms/InstCombine/select.ll b/llvm/test/Transforms/InstCombine/select.ll
index 0168a804239a89..f8067d6eade640 100644
--- a/llvm/test/Transforms/InstCombine/select.ll
+++ b/llvm/test/Transforms/InstCombine/select.ll
@@ -3764,12 +3764,8 @@ exit:
 define i32 @src_and_eq_0_or_xor(i32 %x, i32 %y) {
 ; CHECK-LABEL: @src_and_eq_0_or_xor(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y:%.*]], [[X:%.*]]
-; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[AND]], 0
-; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y]], [[X]]
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y]], [[X]]
-; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[OR]], i32 [[XOR]]
-; CHECK-NEXT:    ret i32 [[COND]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    ret i32 [[XOR]]
 ;
 entry:
   %and = and i32 %y, %x
@@ -3784,12 +3780,8 @@ entry:
 define i32 @src_and_eq_0_xor_or(i32 %x, i32 %y) {
 ; CHECK-LABEL: @src_and_eq_0_xor_or(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y:%.*]], [[X:%.*]]
-; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[AND]], 0
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y]], [[X]]
-; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y]], [[X]]
-; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[XOR]], i32 [[OR]]
-; CHECK-NEXT:    ret i32 [[COND]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    ret i32 [[OR]]
 ;
 entry:
   %and = and i32 %y, %x
@@ -3841,9 +3833,13 @@ entry:
 define i32 @src_and_eq_C_or_xororC(i32 %x, i32 %y, i32 %c) {
 ; CHECK-LABEL: @src_and_eq_C_or_xororC(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y:%.*]], [[X:%.*]]
-; CHECK-NEXT:    [[OR1:%.*]] = or i32 [[XOR]], [[C:%.*]]
-; CHECK-NEXT:    ret i32 [[OR1]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[AND]], [[C:%.*]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[OR1:%.*]] = or i32 [[XOR]], [[C]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[OR]], i32 [[OR1]]
+; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %and = and i32 %y, %x
@@ -3858,9 +3854,13 @@ entry:
 define i32 @src_and_eq_C_or_xorxorC(i32 %x, i32 %y, i32 %c) {
 ; CHECK-LABEL: @src_and_eq_C_or_xorxorC(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y:%.*]], [[X:%.*]]
-; CHECK-NEXT:    [[XOR1:%.*]] = xor i32 [[XOR]], [[C:%.*]]
-; CHECK-NEXT:    ret i32 [[XOR1]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[AND]], [[C:%.*]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[XOR1:%.*]] = xor i32 [[XOR]], [[C]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[OR]], i32 [[XOR1]]
+; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %and = and i32 %y, %x
@@ -3875,10 +3875,14 @@ entry:
 define i32 @src_and_eq_C_xor_OrAndNotC(i32 %x, i32 %y, i32 %c) {
 ; CHECK-LABEL: @src_and_eq_C_xor_OrAndNotC(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
-; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[C:%.*]], -1
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[AND]], [[C:%.*]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[C]], -1
 ; CHECK-NEXT:    [[AND1:%.*]] = and i32 [[OR]], [[NOT]]
-; CHECK-NEXT:    ret i32 [[AND1]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[XOR]], i32 [[AND1]]
+; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %and = and i32 %y, %x
@@ -3917,9 +3921,13 @@ entry:
 define i32 @src_and_eq_C_xor_orxorC(i32 %x, i32 %y, i32 %c) {
 ; CHECK-LABEL: @src_and_eq_C_xor_orxorC(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
-; CHECK-NEXT:    [[XOR1:%.*]] = xor i32 [[OR]], [[C:%.*]]
-; CHECK-NEXT:    ret i32 [[XOR1]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[AND]], [[C:%.*]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[XOR1:%.*]] = xor i32 [[OR]], [[C]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[XOR]], i32 [[XOR1]]
+; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %and = and i32 %y, %x
@@ -3937,11 +3945,8 @@ entry:
 define i32 @src_or_eq_0_and_xor(i32 %x, i32 %y) {
 ; CHECK-LABEL: @src_or_eq_0_and_xor(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
-; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[OR]], 0
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y]], [[X]]
-; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 0, i32 [[XOR]]
-; CHECK-NEXT:    ret i32 [[COND]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    ret i32 [[XOR]]
 ;
 entry:
   %or = or i32 %y, %x
@@ -3956,11 +3961,8 @@ entry:
 define i32 @src_or_eq_0_xor_and(i32 %x, i32 %y) {
 ; CHECK-LABEL: @src_or_eq_0_xor_and(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
-; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[OR]], 0
-; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y]], [[X]]
-; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 0, i32 [[AND]]
-; CHECK-NEXT:    ret i32 [[COND]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    ret i32 [[AND]]
 ;
 entry:
   %or = or i32 %y, %x
@@ -3974,9 +3976,13 @@ entry:
 define i32 @src_or_eq_neg1_and_xor(i32 %x, i32 %y) {
 ; CHECK-LABEL: @src_or_eq_neg1_and_xor(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[TMP0:%.*]] = xor i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[OR]], -1
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[TMP0:%.*]] = xor i32 [[X]], [[Y]]
 ; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[TMP0]], -1
-; CHECK-NEXT:    ret i32 [[NOT]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[AND]], i32 [[NOT]]
+; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %or = or i32 %y, %x
@@ -3991,9 +3997,13 @@ entry:
 define i32 @src_or_eq_neg1_xor_and(i32 %x, i32 %y) {
 ; CHECK-LABEL: @src_or_eq_neg1_xor_and(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[OR]], -1
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y]], [[X]]
 ; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[AND]], -1
-; CHECK-NEXT:    ret i32 [[NOT]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[XOR]], i32 [[NOT]]
+; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %or = or i32 %y, %x
@@ -4008,9 +4018,13 @@ entry:
 define i32 @src_or_eq_C_and_xorC(i32 %x, i32 %y, i32 %c) {
 ; CHECK-LABEL: @src_or_eq_C_and_xorC(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y:%.*]], [[X:%.*]]
-; CHECK-NEXT:    [[XOR1:%.*]] = xor i32 [[XOR]], [[C:%.*]]
-; CHECK-NEXT:    ret i32 [[XOR1]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[OR]], [[C:%.*]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[XOR1:%.*]] = xor i32 [[XOR]], [[C]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[AND]], i32 [[XOR1]]
+; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %or = or i32 %y, %x
@@ -4025,10 +4039,14 @@ entry:
 define i32 @src_or_eq_C_and_andnotxorC(i32 %x, i32 %y, i32 %c) {
 ; CHECK-LABEL: @src_or_eq_C_and_andnotxorC(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[TMP0:%.*]] = xor i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[OR]], [[C:%.*]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[TMP0:%.*]] = xor i32 [[X]], [[Y]]
 ; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[TMP0]], -1
-; CHECK-NEXT:    [[AND1:%.*]] = and i32 [[C:%.*]], [[NOT]]
-; CHECK-NEXT:    ret i32 [[AND1]]
+; CHECK-NEXT:    [[AND1:%.*]] = and i32 [[C]], [[NOT]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[AND]], i32 [[AND1]]
+; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %or = or i32 %y, %x
@@ -4044,9 +4062,13 @@ entry:
 define i32 @src_or_eq_C_xor_xorandC(i32 %x, i32 %y, i32 %c) {
 ; CHECK-LABEL: @src_or_eq_C_xor_xorandC(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y:%.*]], [[X:%.*]]
-; CHECK-NEXT:    [[XOR1:%.*]] = xor i32 [[AND]], [[C:%.*]]
-; CHECK-NEXT:    ret i32 [[XOR1]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[OR]], [[C:%.*]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[XOR1:%.*]] = xor i32 [[AND]], [[C]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[XOR]], i32 [[XOR1]]
+; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %or = or i32 %y, %x
@@ -4061,10 +4083,14 @@ entry:
 define i32 @src_or_eq_C_xor_andnotandC(i32 %x, i32 %y, i32 %c) {
 ; CHECK-LABEL: @src_or_eq_C_xor_andnotandC(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[OR]], [[C:%.*]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y]], [[X]]
 ; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[AND]], -1
-; CHECK-NEXT:    [[AND1:%.*]] = and i32 [[C:%.*]], [[NOT]]
-; CHECK-NEXT:    ret i32 [[AND1]]
+; CHECK-NEXT:    [[AND1:%.*]] = and i32 [[C]], [[NOT]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[XOR]], i32 [[AND1]]
+; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %or = or i32 %y, %x
@@ -4082,9 +4108,13 @@ entry:
 define i32 @src_xor_eq_neg1_and(i32 %x, i32 %y) {
 ; CHECK-LABEL: @src_xor_eq_neg1_and(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[XOR]], -1
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y]], [[X]]
 ; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[OR]], -1
-; CHECK-NEXT:    ret i32 [[NOT]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[AND]], i32 [[NOT]]
+; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %xor = xor i32 %y, %x
@@ -4117,9 +4147,13 @@ entry:
 define i32 @src_xor_eq_C_and_xororC(i32 %x, i32 %y, i32 %c) {
 ; CHECK-LABEL: @src_xor_eq_C_and_xororC(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
-; CHECK-NEXT:    [[XOR1:%.*]] = xor i32 [[OR]], [[C:%.*]]
-; CHECK-NEXT:    ret i32 [[XOR1]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[XOR]], [[C:%.*]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[XOR1:%.*]] = xor i32 [[OR]], [[C]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[AND]], i32 [[XOR1]]
+; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %xor = xor i32 %y, %x
@@ -4134,10 +4168,14 @@ entry:
 define i32 @src_xor_eq_C_and_andornotC(i32 %x, i32 %y, i32 %c) {
 ; CHECK-LABEL: @src_xor_eq_C_and_andornotC(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y:%.*]], [[X:%.*]]
-; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[C:%.*]], -1
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[XOR]], [[C:%.*]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[OR:%.*]] = or i32 [[Y]], [[X]]
+; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[C]], -1
 ; CHECK-NEXT:    [[AND1:%.*]] = and i32 [[OR]], [[NOT]]
-; CHECK-NEXT:    ret i32 [[AND1]]
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP]], i32 [[AND]], i32 [[AND1]]
+; CHECK-NEXT:    ret i32 [[COND]]
 ;
 entry:
   %xor = xor i32 %y, %x
@@ -4153,9 +4191,13 @@ entry:
 define i32 @src_xor_eq_C_or_xorandC(i32 %x, i32 %y, i32 %c) {
 ; CHECK-LABEL: @src_xor_eq_C_or_xorandC(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[AND:%.*]] = and i32 [[Y:%.*]], [[X:%.*]]
-; CHECK-NEXT:    [[XOR1:%.*]] = xor i32 [[AND]], [[C:%.*]]
-; CHECK-NEXT:    ret i32 [[XOR1]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[Y:%.*]], [[X:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[XOR]], [[C:...
[truncated]

@goldsteinn
Copy link
Contributor Author

NB: If this has no affect on opt benchmarks I'm going to instead propose just dropping it.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 5, 2025

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command: 
git-clang-format --diff c56b74315f57acb1b285ddc77b07031b773549b7 ee59139acb3995a7688e77ec35b87f408dc31a68 --extensions cpp -- llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
View the diff from clang-format here. 
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 2838d45fcc..948ec3360c 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -1846,7 +1846,7 @@ static Instruction *foldSelectICmpEq(SelectInst &SI, ICmpInst *ICI,
   if (match(CmpRHS, m_Zero())) {
     // (X & Y) == 0 ? X |/^/+ Y : X |/^/+ Y -> X |/^/+ Y (false arm)
     // `(X & Y) == 0` implies no common bits which means:
-	// `X ^ Y == X | Y == X + Y`
+    // `X ^ Y == X | Y == X + Y`
     // https://alive2.llvm.org/ce/z/jjcduh
     if (match(CmpLHS, m_And(m_Value(X), m_Value(Y)))) {
       auto MatchAddOrXor =
@@ -1859,10 +1859,10 @@ static Instruction *foldSelectICmpEq(SelectInst &SI, ICmpInst *ICI,
 
     // (X | Y) == 0 ? X Op0 Y : X Op1 Y -> X Op1 Y
     // For any `Op0` and `Op1` that are zero when `X` and `Y` are zero.
-	// https://alive2.llvm.org/ce/z/azHzBW
+    // https://alive2.llvm.org/ce/z/azHzBW
     if (match(CmpLHS, m_Or(m_Value(X), m_Value(Y))) &&
         (match(TrueVal, m_c_BinOp(m_Specific(X), m_Specific(Y))) ||
-		 // In true arm we can also accept just `0`.
+         // In true arm we can also accept just `0`.
          match(TrueVal, m_Zero())) &&
         match(FalseVal, m_c_BinOp(m_Specific(X), m_Specific(Y)))) {
       auto IsOpcZeroWithZeros = [](Value *V) {

@dtcxzyw dtcxzyw mentioned this pull request Jan 5, 2025
Closed
@goldsteinn
Copy link
Contributor Author

This has no affect on opt benchmark. @nikic, if you want to drop the entire function LMK.

nikic
nikic reviewed Jan 5, 2025
View reviewed changes
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
return IC.replaceInstUsesWith(
SI, IC.Builder.CreateBinOp(BO->getOpcode(), BO->getOperand(0),
BO->getOperand(1)));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we handle this case by changing

llvm-project/llvm/lib/Analysis/InstructionSimplify.cpp

Lines 4596 to 4612 in 3321c2d

/// Try to simplify a select instruction when its condition operand is an
/// integer equality or floating-point equivalence comparison.
static Value *simplifySelectWithEquivalence(Value *CmpLHS, Value *CmpRHS,
Value *TrueVal, Value *FalseVal,
const SimplifyQuery &Q,
unsigned MaxRecurse) {
if (simplifyWithOpReplaced(FalseVal, CmpLHS, CmpRHS, Q.getWithoutUndef(),
/* AllowRefinement */ false,
/* DropFlags */ nullptr, MaxRecurse) == TrueVal)
return FalseVal;
if (simplifyWithOpReplaced(TrueVal, CmpLHS, CmpRHS, Q,
/* AllowRefinement */ true,
/* DropFlags */ nullptr, MaxRecurse) == FalseVal)
return FalseVal;
return nullptr;
}
to compare both simplified values instead? Currently we simplify one and compare against the original other. If we compared both simplified values, I think we should be able to handle this pattern without any specialized code.

nikic
nikic reviewed Jan 5, 2025
View reviewed changes
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
}
// (X | Y) == 0 ? X Op0 Y : X Op1 Y -> X Op1 Y
// For any `Op0` and `Op1` that are zero when `X` and `Y` are zero.
// https://alive2.llvm.org/ce/z/azHzBW
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In conjunction with my suggestion for the eq case, a principled way to handle this would be to extend

llvm-project/llvm/lib/Analysis/InstructionSimplify.cpp

Lines 4711 to 4721 in 3321c2d

// select((X | Y) == 0 ? X : 0) --> 0 (commuted 2 ways)
if (match(CmpLHS, m_Or(m_Value(X), m_Value(Y))) &&
match(CmpRHS, m_Zero())) {
// (X | Y) == 0 implies X == 0 and Y == 0.
if (Value *V = simplifySelectWithEquivalence(X, CmpRHS, TrueVal, FalseVal,
Q, MaxRecurse))
return V;
if (Value *V = simplifySelectWithEquivalence(Y, CmpRHS, TrueVal, FalseVal,
Q, MaxRecurse))
return V;
}
and simplifyWithOpReplaced to support multiple replacements at the same time, instead of trying to replace X and Y with 0 individually. (Whether this is feasible depends on the compile-time impact of course.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for X | Y == 0, but not the X & Y == 0 case.

I think best way forward is add that simplification with follow up to replace X & Y == 0 ? X ^/+ Y : ... with X & Y == 0 ? X | Y : .... I don't think the full fold of X & Y == 0 : X | Y : X ^/|/+ Y really exists, but it would be just a single line to add if we decide its worthwhile.

goldsteinn added a commit to goldsteinn/llvm-project that referenced this pull request Jan 5, 2025
@goldsteinn
[InstSimplify] Use multi-op replacement when simplify select
0c2ed79 
In the case of `select X | Y == 0 :...` or `select X & Y == -1 : ...`
we can do more simplifications by trying to replace both `X` and `Y`
with the respective constant at once.

Handles some cases for llvm#121672 more generically.
@goldsteinn goldsteinn mentioned this pull request Jan 5, 2025
Merged
@dtcxzyw dtcxzyw mentioned this pull request Jan 6, 2025
Open
goldsteinn added a commit to goldsteinn/llvm-project that referenced this pull request Jan 6, 2025
@goldsteinn
[InstSimplify] Use multi-op replacement when simplify select
c63af84 
In the case of `select X | Y == 0 :...` or `select X & Y == -1 : ...`
we can do more simplifications by trying to replace both `X` and `Y`
with the respective constant at once.

Handles some cases for llvm#121672 more generically.
goldsteinn added a commit to goldsteinn/llvm-project that referenced this pull request Jan 6, 2025
@goldsteinn
[InstSimplify] Use multi-op replacement when simplify select
6376b4c 
In the case of `select X | Y == 0 :...` or `select X & Y == -1 : ...`
we can do more simplifications by trying to replace both `X` and `Y`
with the respective constant at once.

Handles some cases for llvm#121672 more generically.
goldsteinn added a commit that referenced this pull request Jan 7, 2025
@goldsteinn
[InstSimplify] Use multi-op replacement when simplify select (#121708)
6192faf 
- **[InstSimplify] Refactor `simplifyWithOpsReplaced` to allow multiple
replacements; NFC**
- **[InstSimplify] Use multi-op replacement when simplify `select`**

In the case of `select X | Y == 0 :...` or `select X & Y == -1 : ...`
we can do more simplifications by trying to replace both `X` and `Y`
with the respective constant at once.

Handles some cases for #121672
more generically.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic left review comments

@dtcxzyw dtcxzyw Awaiting requested review from dtcxzyw

Assignees
No one assigned
Labels
llvm:instcombine llvm:transforms
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@goldsteinn @llvmbot @nikic