[InstCombine] Fold icmp in select to smin #87157

Krishna-13-cyber · 2024-03-30T14:55:45Z

Intends to solve #85844
Alive2 Proofs: https://alive2.llvm.org/ce/z/FRNgbc

llvmbot · 2024-03-30T14:56:16Z

@llvm/pr-subscribers-llvm-transforms

Author: Krishna Narayanan (Krishna-13-cyber)

Changes

Intends to solve #85844

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp (+22)
(modified) llvm/test/Transforms/InstCombine/icmp-select.ll (+13)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 9ab2bd8f70aa15..4e58ba5f01df1c 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -1687,6 +1687,25 @@ static Value *foldSelectInstWithICmpConst(SelectInst &SI, ICmpInst *ICI,
   return nullptr;
 }
 
+static Value *foldSelectInstWithICmpOr(SelectInst &SI, ICmpInst *ICI,
+                                       InstCombiner::BuilderTy &Builder) {
+  /* a > -1 ? 1 : (a | 3) --> smin(1, a | 3) */
+  Type *Ty = SI.getType();
+  Value *TVal = SI.getTrueValue();
+  Value *FVal = SI.getFalseValue();
+  Constant *One = ConstantInt::get(Ty, 1);
+  CmpInst::Predicate Pred = ICI->getPredicate();
+  Value *A = ICI->getOperand(0);
+  const APInt *Cmp;
+
+  if (!match(TVal, m_One()) || !match(FVal, m_Or(m_Value(A), m_SpecificInt(3))))
+    return nullptr;
+
+  if (Pred == ICmpInst::ICMP_SGT && *Cmp == -1 && match(TVal, m_One()) &&
+      match(FVal, m_Or(m_Value(A), m_SpecificInt(3))))
+    return Builder.CreateBinaryIntrinsic(Intrinsic::smin, One, FVal);
+}
+
 /// Visit a SelectInst that has an ICmpInst as its first operand.
 Instruction *InstCombinerImpl::foldSelectInstWithICmp(SelectInst &SI,
                                                       ICmpInst *ICI) {
@@ -1700,6 +1719,9 @@ Instruction *InstCombinerImpl::foldSelectInstWithICmp(SelectInst &SI,
   if (Value *V = foldSelectInstWithICmpConst(SI, ICI, Builder))
     return replaceInstUsesWith(SI, V);
 
+  if (Value *V = foldSelectInstWithICmpOr(SI, ICI, Builder))
+    return replaceInstUsesWith(SI, V);
+
   if (Value *V = canonicalizeClampLike(SI, *ICI, Builder))
     return replaceInstUsesWith(SI, V);
 
diff --git a/llvm/test/Transforms/InstCombine/icmp-select.ll b/llvm/test/Transforms/InstCombine/icmp-select.ll
index 59d2a1b165c0f8..80341278b58f3a 100644
--- a/llvm/test/Transforms/InstCombine/icmp-select.ll
+++ b/llvm/test/Transforms/InstCombine/icmp-select.ll
@@ -628,3 +628,16 @@ define i1 @icmp_slt_select(i1 %cond, i32 %a, i32 %b) {
   %res = icmp slt i32 %lhs, %rhs
   ret i1 %res
 }
+
+define i8 @icmp_slt_select_or(i8 %inl) {
+; CHECK-LABEL: @icmp_slt_select_or(
+; CHECK-NEXT:    [[OR:%.*]] = or i8 [[INL:%.*]], 3
+; CHECK-NEXT:    [[CMP:%.*]] = icmp sgt i8 [[INL]], -1
+; CHECK-NEXT:    [[S:%.*]] = select i1 [[CMP]], i8 1, i8 [[OR]]
+; CHECK-NEXT:    ret i8 [[S]]
+;
+  %or = or i8 %inl, 3
+  %cmp = icmp sgt i8 %inl, -1
+  %s = select i1 %cmp, i8 1, i8 %or
+  ret i8 %s
+}

XChy

The issue is a specifc case, you need to provide a generalized pattern at least. See also https://llvm.org/docs/InstCombineContributorGuide.html#generalization

Krishna-13-cyber · 2024-03-30T15:06:52Z

The issue is a specifc case, you need to provide a generalized pattern at least. See also https://llvm.org/docs/InstCombineContributorGuide.html#generalization

Got it, I will make the required changes.

goldsteinn · 2024-04-03T18:39:34Z

Your proofs are incomplete, you only prove the specific constants you chose (and in fact your code does not hold for value <= 0).
For more info on how to write robust proofs see: https://llvm.org/docs/InstCombineContributorGuide.html#proofs
Here is an example: https://alive2.llvm.org/ce/z/sm9_gy

goldsteinn · 2024-04-03T18:39:58Z

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

+                                       InstCombiner::BuilderTy &Builder) {
+
+  // a > -1 ? 1 : (a | value) --> smin(1, a | value)
+  // a >= 0 ? 1 : (a | value) --> smin(1, a | value)


You don't need to handle a >= 0, it is canonicalized to a > -1.

goldsteinn · 2024-04-03T18:40:41Z

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

+  Constant *Zero = Constant::getNullValue(Ty);
+  CmpInst::Predicate Pred = ICI->getPredicate();
+
+  if (!match(B, m_APIntAllowUndef(Cmp)))


If you are going to allow undef, please add a vector test with some undef elements.

~~But either way, this should just be if(!match(B, m_AllOnes())~~

goldsteinn · 2024-04-03T18:42:04Z

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

+
+  // Swap TVal, FVal for Inverse
+  if (Pred == ICmpInst::ICMP_SLT || Pred == ICmpInst::ICMP_SLE)
+    std::swap(TVal, FVal);


Think you are missing a Pred = ICmpInst::getInversePred(Pred) here too.

goldsteinn · 2024-04-03T18:42:29Z

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

+    return nullptr;
+
+  if (((Pred == ICmpInst::ICMP_SGT && *Cmp == -1) ||
+       (Pred == ICmpInst::ICMP_SGE && *Cmp == 0)))


~~As mentioned above, no need for the SGE / *Cmp==0 case.~~
You need it for the swapped slt case.

goldsteinn · 2024-04-03T18:44:12Z

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

+static Value *foldSelectInstWithICmpOr(SelectInst &SI, ICmpInst *ICI,
+                                       InstCombiner::BuilderTy &Builder) {
+
+  // a > -1 ? 1 : (a | value) --> smin(1, a | value)


comment should indicate value must be strictly positive.

goldsteinn · 2024-04-03T18:45:12Z

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

+  if (match(TVal, m_Zero()))
+    return Builder.CreateBinaryIntrinsic(Intrinsic::smin, Zero, FVal);
+  if (match(TVal, m_AllOnes()))
+    return Builder.CreateBinaryIntrinsic(Intrinsic::smin, NegOne, FVal);


This could be simplified to:

if(match(TVal, m_One()) || match(TVal, m_AllOnes()) || match(TVal, m_Zero())) { return Builder.Create...(smin, TVal, FVal); }

Also your comment / proofs only seem to cover the TVal == 1 case, can you please update them to cover the other two cases.

I think the same link has tests with TVal for -1, 1 and 0. I will check again and update that accordingly.

goldsteinn · 2024-04-03T18:50:00Z

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

+      !match(FVal, m_Or(m_Value(A), m_StrictlyPositive())))
+    return nullptr;
+
+  if (((Pred == ICmpInst::ICMP_SGT && *Cmp == -1) ||


Cmp->isAllOnes() and Cmp->isZero()

Krishna-13-cyber · 2024-04-04T17:54:33Z

Your proofs are incomplete, you only prove the specific constants you chose (and in fact your code does not hold for value <= 0). For more info on how to write robust proofs see: https://llvm.org/docs/InstCombineContributorGuide.html#proofs Here is an example: https://alive2.llvm.org/ce/z/sm9_gy

Thanks a lot for the review, yes I did refer the guide, incorporated these proofs as well.
Actually this fold does'nt go well for negative values https://alive2.llvm.org/ce/z/geeSb6, due to which I constrained it to strictly positive. I am trying to this generalise for neg values as well by some means.
We have something like this https://alive2.llvm.org/ce/z/NrmVs8 which goes on similar basis for neg values(logic wise).

goldsteinn · 2024-04-12T17:53:55Z

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

+      !(match(FVal, m_Or(m_Value(A), m_Value(C)))))
+    return nullptr;
+
+  if (!match(C, m_StrictlyPositive()))


Whats the point if using C as an intermediate here?

Yes, there was no specific reason/point to do this.

if (....!(match(FVal, m_Or(m_Value(A), m_Value(C))))) return nullptr; if (!match(C, m_StrictlyPositive())) return nullptr;

if (...... !(match(FVal, m_Or(m_Value(A), m_StrictlyPositive()))))

Either of them failed to pass the llvm.assume test cases as the expression don't fold, so tried using the earlier approach.
As this fold is only applicable for positive values, we cannot match any other values other than this as it would lead to invalid transform.
Not sure what constraint or needful I am missing out to handle these generic testcases, would need some assistance in this.

m_StrictlyPositive matches an constant that is positive. To check certain bits in a non-constant value use the ValueTracking interface. In this case you would use isKnownStrictlyPositive.

Fold icmp in select to smin

394befb

Krishna-13-cyber requested a review from nikic as a code owner March 30, 2024 14:55

Krishna-13-cyber marked this pull request as draft March 30, 2024 14:55

llvmbot added the llvm:transforms label Mar 30, 2024

Update test diffs

bf96ccc

XChy requested changes Mar 30, 2024

View reviewed changes

Update with test cases

54ab631

goldsteinn requested a review from dtcxzyw April 3, 2024 18:33

goldsteinn reviewed Apr 3, 2024

View reviewed changes

dtcxzyw marked this pull request as ready for review April 4, 2024 07:14

Update with review and tests

9532982

goldsteinn reviewed Apr 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[InstCombine] Fold icmp in select to smin #87157

[InstCombine] Fold icmp in select to smin #87157

Krishna-13-cyber commented Mar 30, 2024 •

edited

llvmbot commented Mar 30, 2024

XChy left a comment

Krishna-13-cyber commented Mar 30, 2024

goldsteinn commented Apr 3, 2024

goldsteinn Apr 3, 2024

goldsteinn Apr 3, 2024

goldsteinn Apr 3, 2024 •

edited

goldsteinn Apr 3, 2024 •

edited

goldsteinn Apr 3, 2024 •

edited

goldsteinn Apr 3, 2024

goldsteinn Apr 3, 2024

goldsteinn Apr 3, 2024

Krishna-13-cyber Apr 4, 2024

goldsteinn Apr 3, 2024

Krishna-13-cyber commented Apr 4, 2024

goldsteinn Apr 12, 2024

Krishna-13-cyber Apr 14, 2024

goldsteinn Apr 16, 2024

[InstCombine] Fold icmp in select to smin #87157

Are you sure you want to change the base?

[InstCombine] Fold icmp in select to smin #87157

Conversation

Krishna-13-cyber commented Mar 30, 2024 • edited

llvmbot commented Mar 30, 2024

XChy left a comment

Choose a reason for hiding this comment

Krishna-13-cyber commented Mar 30, 2024

goldsteinn commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goldsteinn Apr 3, 2024 • edited

Choose a reason for hiding this comment

goldsteinn Apr 3, 2024 • edited

Choose a reason for hiding this comment

goldsteinn Apr 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krishna-13-cyber commented Apr 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krishna-13-cyber commented Mar 30, 2024 •

edited

goldsteinn Apr 3, 2024 •

edited

goldsteinn Apr 3, 2024 •

edited

goldsteinn Apr 3, 2024 •

edited