[InstCombine] Respect samesign flag in foldAndOrOfICmpsWithConstEq

[dtcxzyw]

In 5dbfca3 we assume that RHS is poison implies LHS is also poison. It doesn't hold after introducing samesign flag. This patch treats this case as logical and swap operands iff RHS has samesign flag and the original expression is a logical and/or.

Alternative solution 1: just drop samesign flag in RHS after folding.
Alternative solution 2: reject this fold if RHS contains samesign flag.

Close #112467.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

In 5dbfca3 we assume that RHS is poison implies LHS is also poison. It doesn't hold after introducing samesign flag. This patch treats this case as logical and swap operands iff RHS has samesign flag and the original expression is a logical and/or.

Alternative solution 1: just drop samesign flag in RHS after folding.
Alternative solution 2: reject this fold if RHS contains samesign flag.

Close #112467.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp (+17-8)
• (modified) llvm/test/Transforms/InstCombine/and-or-icmps.ll (+13)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index 64bee4ab974ede..22728bc8abda46 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -1261,7 +1261,8 @@ Value *InstCombinerImpl::foldEqOfParts(ICmpInst *Cmp0, ICmpInst *Cmp1,
 static Value *foldAndOrOfICmpsWithConstEq(ICmpInst *Cmp0, ICmpInst *Cmp1,
                                           bool IsAnd, bool IsLogical,
                                           InstCombiner::BuilderTy &Builder,
-                                          const SimplifyQuery &Q) {
+                                          const SimplifyQuery &Q,
+                                          bool SwapOpOrder) {
   // Match an equality compare with a non-poison constant as Cmp0.
   // Also, give up if the compare can be constant-folded to avoid looping.
   ICmpInst::Predicate Pred0;
@@ -1295,9 +1296,14 @@ static Value *foldAndOrOfICmpsWithConstEq(ICmpInst *Cmp0, ICmpInst *Cmp1,
       return nullptr;
     SubstituteCmp = Builder.CreateICmp(Pred1, Y, C);
   }
-  if (IsLogical)
-    return IsAnd ? Builder.CreateLogicalAnd(Cmp0, SubstituteCmp)
-                 : Builder.CreateLogicalOr(Cmp0, SubstituteCmp);
+  if (IsLogical) {
+    Value *LHS = Cmp0;
+    Value *RHS = SubstituteCmp;
+    if (SwapOpOrder)
+      std::swap(LHS, RHS);
+    return IsAnd ? Builder.CreateLogicalAnd(LHS, RHS)
+                 : Builder.CreateLogicalOr(LHS, RHS);
+  }
   return Builder.CreateBinOp(IsAnd ? Instruction::And : Instruction::Or, Cmp0,
                              SubstituteCmp);
 }
@@ -3363,13 +3369,16 @@ Value *InstCombinerImpl::foldAndOrOfICmps(ICmpInst *LHS, ICmpInst *RHS,
                                                   /*IsLogical*/ false, Builder))
     return V;
 
-  if (Value *V =
-          foldAndOrOfICmpsWithConstEq(LHS, RHS, IsAnd, IsLogical, Builder, Q))
+  if (Value *V = foldAndOrOfICmpsWithConstEq(LHS, RHS, IsAnd, IsLogical,
+                                             Builder, Q, /*SwapOpOrder=*/false))
     return V;
   // We can convert this case to bitwise and, because both operands are used
-  // on the LHS, and as such poison from both will propagate.
+  // on the LHS, and as such poison from both will propagate (unless RHS has
+  // samesign flag).
   if (Value *V = foldAndOrOfICmpsWithConstEq(RHS, LHS, IsAnd,
-                                             /*IsLogical*/ false, Builder, Q))
+                                             /*IsLogical=*/IsLogical &&
+                                                 RHS->hasSameSign(),
+                                             Builder, Q, /*SwapOpOrder=*/true))
     return V;
 
   if (Value *V = foldIsPowerOf2OrZero(LHS, RHS, IsAnd, Builder, *this))
diff --git a/llvm/test/Transforms/InstCombine/and-or-icmps.ll b/llvm/test/Transforms/InstCombine/and-or-icmps.ll
index eb4723c86542de..a02db166acf5da 100644
--- a/llvm/test/Transforms/InstCombine/and-or-icmps.ll
+++ b/llvm/test/Transforms/InstCombine/and-or-icmps.ll
@@ -3416,3 +3416,16 @@ define i1 @and_ugt_to_mask_off_by_one(i8 %x) {
   %and2 = and i1 %cmp, %cmp2
   ret i1 %and2
 }
+
+define i1 @logical_or_icmp_eq_const_samesign(i8 %x, i8 %y) {
+; CHECK-LABEL: @logical_or_icmp_eq_const_samesign(
+; CHECK-NEXT:    [[CMPEQ:%.*]] = icmp samesign ne i8 [[X:%.*]], 0
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp slt i8 [[Y:%.*]], 0
+; CHECK-NEXT:    [[R:%.*]] = select i1 [[TMP1]], i1 true, i1 [[CMPEQ]]
+; CHECK-NEXT:    ret i1 [[R]]
+;
+  %cmp = icmp sgt i8 %x, %y
+  %cmpeq = icmp samesign ne i8 %x, 0
+  %r = select i1 %cmp, i1 true, i1 %cmpeq
+  ret i1 %r
+}

[dtcxzyw]

Unfortunately this approach doesn't fix the following case:

define i1 @sge_and_max_logical(i8 %x, i8 %y)  {
  %cmp = icmp sge i8 %x, %y
  %cmpeq = icmp samesign eq i8 %x, 127
  %r = select i1 %cmp, i1 %cmpeq, i1 false
  ret i1 %r
}

[dtcxzyw]

Closed in favor of #112489.