[InstCombine] Folding `(icmp eq/ne (and X, -P2), INT_MIN)` #110880

goldsteinn · 2024-10-02T16:21:03Z

[InstCombine] Add tests for folding (icmp eq/ne (and X, -P2), INT_MIN); NFC
[InstCombine] Folding (icmp eq/ne (and X, -P2), INT_MIN)

Folds to (icmp slt/sge X, (INT_MIN + P2))

Proofs: https://alive2.llvm.org/ce/z/vpNFY5

…N)`; NFC

Folds to `(icmp slt/sge X, (INT_MIN + P2))` Proofs: https://alive2.llvm.org/ce/z/vpNFY5

llvmbot · 2024-10-02T16:21:41Z

@llvm/pr-subscribers-llvm-transforms

Author: None (goldsteinn)

Changes

[InstCombine] Add tests for folding (icmp eq/ne (and X, -P2), INT_MIN); NFC
[InstCombine] Folding (icmp eq/ne (and X, -P2), INT_MIN)

Folds to (icmp slt/sge X, (INT_MIN + P2))

Proofs: https://alive2.llvm.org/ce/z/vpNFY5

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp (+16)
(added) llvm/test/Transforms/InstCombine/icmp-signmask.ll (+54)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index b0771ffcc38a83..5e1d9a1c52b2f2 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -5015,6 +5015,22 @@ Instruction *InstCombinerImpl::foldICmpBinOp(ICmpInst &I,
     }
   }
 
+  // (icmp eq/ne (X, -P2), INT_MIN)
+  //	-> (icmp slt/sge X, INT_MIN + P2)
+  if (ICmpInst::isEquality(Pred) && BO0 &&
+      match(I.getOperand(1), m_SignMask())) {
+    Value *X;
+    if (match(BO0, m_And(m_Value(X), m_CheckedInt([](const APInt &C) {
+                           return C.isZero() || C.isNegatedPowerOf2();
+                         })))) {
+      // Will Constant fold.
+      Value *NewC = Builder.CreateSub(I.getOperand(1), BO0->getOperand(1));
+      return new ICmpInst(Pred == ICmpInst::ICMP_EQ ? ICmpInst::ICMP_SLT
+                                                    : ICmpInst::ICMP_SGE,
+                          X, NewC);
+    }
+  }
+
   {
     // Similar to above: an unsigned overflow comparison may use offset + mask:
     // ((Op1 + C) & C) u<  Op1 --> Op1 != 0
diff --git a/llvm/test/Transforms/InstCombine/icmp-signmask.ll b/llvm/test/Transforms/InstCombine/icmp-signmask.ll
new file mode 100644
index 00000000000000..5424f7d7e8021f
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/icmp-signmask.ll
@@ -0,0 +1,54 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+
+define i1 @cmp_x_and_negp2_with_eq(i8 %x) {
+; CHECK-LABEL: @cmp_x_and_negp2_with_eq(
+; CHECK-NEXT:    [[R:%.*]] = icmp slt i8 [[X:%.*]], -126
+; CHECK-NEXT:    ret i1 [[R]]
+;
+  %andx = and i8 %x, -2
+  %r = icmp eq i8 %andx, 128
+  ret i1 %r
+}
+
+define i1 @cmp_x_and_negp2_with_eq_fail_not_signmask(i8 %x) {
+; CHECK-LABEL: @cmp_x_and_negp2_with_eq_fail_not_signmask(
+; CHECK-NEXT:    [[ANDX:%.*]] = and i8 [[X:%.*]], -2
+; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[ANDX]], -124
+; CHECK-NEXT:    ret i1 [[R]]
+;
+  %andx = and i8 %x, -2
+  %r = icmp eq i8 %andx, 132
+  ret i1 %r
+}
+
+define <2 x i1> @cmp_x_and_negp2_with_ne(<2 x i8> %x) {
+; CHECK-LABEL: @cmp_x_and_negp2_with_ne(
+; CHECK-NEXT:    [[R:%.*]] = icmp sgt <2 x i8> [[X:%.*]], <i8 -121, i8 -113>
+; CHECK-NEXT:    ret <2 x i1> [[R]]
+;
+  %andx = and <2 x i8> %x, <i8 -8, i8 -16>
+  %r = icmp ne <2 x i8> %andx, <i8 128, i8 128>
+  ret <2 x i1> %r
+}
+
+define <2 x i1> @cmp_x_and_negp2_with_ne_or_z(<2 x i8> %x) {
+; CHECK-LABEL: @cmp_x_and_negp2_with_ne_or_z(
+; CHECK-NEXT:    [[R:%.*]] = icmp sge <2 x i8> [[X:%.*]], <i8 -128, i8 -112>
+; CHECK-NEXT:    ret <2 x i1> [[R]]
+;
+  %andx = and <2 x i8> %x, <i8 0, i8 -16>
+  %r = icmp ne <2 x i8> %andx, <i8 128, i8 128>
+  ret <2 x i1> %r
+}
+
+define <2 x i1> @cmp_x_and_negp2_with_ne_fail_not_p2(<2 x i8> %x) {
+; CHECK-LABEL: @cmp_x_and_negp2_with_ne_fail_not_p2(
+; CHECK-NEXT:    [[ANDX:%.*]] = and <2 x i8> [[X:%.*]], <i8 -8, i8 -15>
+; CHECK-NEXT:    [[R:%.*]] = icmp ne <2 x i8> [[ANDX]], <i8 -128, i8 -128>
+; CHECK-NEXT:    ret <2 x i1> [[R]]
+;
+  %andx = and <2 x i8> %x, <i8 -8, i8 -15>
+  %r = icmp ne <2 x i8> %andx, <i8 128, i8 128>
+  ret <2 x i1> %r
+}

dtcxzyw

LG

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

nikic

LG

nikic · 2024-10-03T16:45:27Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

+  //	-> (icmp slt/sge X, INT_MIN + P2)
+  if (ICmpInst::isEquality(Pred) && BO0 &&
+      match(I.getOperand(1), m_SignMask())) {
+    Value *X;


There's already a Value *X declaration above, so you can drop this one and merge both ifs.

Folds to `(icmp slt/sge X, (INT_MIN + P2))` Proofs: https://alive2.llvm.org/ce/z/vpNFY5 Closes llvm#110880

vitalybuka · 2024-10-04T20:19:37Z

@eugenis

Looks like this breaks msan
https://lab.llvm.org/buildbot/#/builders/164/builds/3279

And I mean msan itself, not the code.
Similar bot on released compiler, and same code, does not repor https://lab.llvm.org/buildbot/#/builders/169

goldsteinn · 2024-10-04T20:39:51Z

@eugenis

Looks like this breaks msan https://lab.llvm.org/buildbot/#/builders/164/builds/3279

And I mean msan itself, not the code. Similar bot on released compiler, and same code, does not repor https://lab.llvm.org/buildbot/#/builders/169

Hmm? As in MSAN no longer works correctly when compiled with this PR?

vitalybuka · 2024-10-04T20:47:04Z

@eugenis
Looks like this breaks msan https://lab.llvm.org/buildbot/#/builders/164/builds/3279
And I mean msan itself, not the code. Similar bot on released compiler, and same code, does not repor https://lab.llvm.org/buildbot/#/builders/169

Hmm? As in MSAN no longer works correctly when compiled with this PR?

Yes, I bisected locally..
Reports looks wrong https://lab.llvm.org/buildbot/#/builders/164/builds/3279/steps/14/logs/stdio

goldsteinn · 2024-10-04T20:48:58Z

@eugenis
Looks like this breaks msan https://lab.llvm.org/buildbot/#/builders/164/builds/3279
And I mean msan itself, not the code. Similar bot on released compiler, and same code, does not repor https://lab.llvm.org/buildbot/#/builders/169

Hmm? As in MSAN no longer works correctly when compiled with this PR?

Yes, I bisected locally.. Reports looks wrong https://lab.llvm.org/buildbot/#/builders/164/builds/3279/steps/14/logs/stdio

Okay, might just be this PR is buggy and causes miscompiles, although its pretty simple and I don't see it.

eugenis · 2024-10-04T20:59:46Z

This is a problem with MSan handling of signed comparison - it is not entirely correct as a compromise for performance and code size. Do you mind reverting the change while we figure out how to fix MSan?

eugenis · 2024-10-04T21:21:30Z

I can confirm that -msan-handle-icmp-exact=1 avoids this issue, and increases .text size of the clang binary by 2.9%.

eugenis · 2024-10-04T21:25:25Z

#111212 to fix MSan.

…111236) Reverts #110880 because of exposed issue is Msan instrumentation #111212. This reverts commit a646436.

…lvm#111236) Reverts llvm#110880 because of exposed issue is Msan instrumentation llvm#111212. This reverts commit a646436.

goldsteinn added 2 commits October 2, 2024 11:15

[InstCombine] Add tests for folding `(icmp eq/ne (and X, -P2), INT_MI…

2aafd94

…N)`; NFC

[InstCombine] Folding (icmp eq/ne (and X, -P2), INT_MIN)

4e51249

Folds to `(icmp slt/sge X, (INT_MIN + P2))` Proofs: https://alive2.llvm.org/ce/z/vpNFY5

goldsteinn requested a review from nikic as a code owner October 2, 2024 16:21

llvmbot added the llvm:transforms label Oct 2, 2024

goldsteinn changed the title ~~goldsteinn/fold icmp and signmask~~ [InstCombine] Folding (icmp eq/ne (and X, -P2), INT_MIN) Oct 2, 2024

goldsteinn requested a review from dtcxzyw October 2, 2024 16:21

goldsteinn mentioned this pull request Oct 2, 2024

[InstCombine] Decompose more icmps into masks #110836

Merged

nikic mentioned this pull request Oct 2, 2024

Task submission dtcxzyw/llvm-opt-benchmark#1312

Open

dtcxzyw mentioned this pull request Oct 2, 2024

pre-commit: PR110880 dtcxzyw/llvm-opt-benchmark#1425

Closed

Fixup failing test

c00badd

dtcxzyw approved these changes Oct 3, 2024

View reviewed changes

nikic reviewed Oct 3, 2024

View reviewed changes

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp Outdated Show resolved Hide resolved

Better matcher

4dc758f

nikic approved these changes Oct 3, 2024

View reviewed changes

goldsteinn closed this in a646436 Oct 3, 2024

xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024

[InstCombine] Folding (icmp eq/ne (and X, -P2), INT_MIN)

3602dcd

Folds to `(icmp slt/sge X, (INT_MIN + P2))` Proofs: https://alive2.llvm.org/ce/z/vpNFY5 Closes llvm#110880

eugenis mentioned this pull request Oct 4, 2024

[msan] Better handling of signed compare #111212

Closed

vitalybuka mentioned this pull request Oct 5, 2024

Revert "[InstCombine] Folding (icmp eq/ne (and X, -P2), INT_MIN)" #111236

Merged

vitalybuka added a commit that referenced this pull request Oct 5, 2024

Revert "[InstCombine] Folding (icmp eq/ne (and X, -P2), INT_MIN)" (#…

574266c

…111236) Reverts #110880 because of exposed issue is Msan instrumentation #111212. This reverts commit a646436.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[InstCombine] Folding `(icmp eq/ne (and X, -P2), INT_MIN)` #110880

[InstCombine] Folding `(icmp eq/ne (and X, -P2), INT_MIN)` #110880

goldsteinn commented Oct 2, 2024 •

edited

Loading

llvmbot commented Oct 2, 2024

dtcxzyw left a comment

nikic left a comment

nikic Oct 3, 2024

vitalybuka commented Oct 4, 2024

goldsteinn commented Oct 4, 2024

vitalybuka commented Oct 4, 2024 •

edited

Loading

goldsteinn commented Oct 4, 2024

eugenis commented Oct 4, 2024

eugenis commented Oct 4, 2024

eugenis commented Oct 4, 2024

[InstCombine] Folding (icmp eq/ne (and X, -P2), INT_MIN) #110880

[InstCombine] Folding (icmp eq/ne (and X, -P2), INT_MIN) #110880

Conversation

goldsteinn commented Oct 2, 2024 • edited Loading

llvmbot commented Oct 2, 2024

dtcxzyw left a comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

nikic Oct 3, 2024

Choose a reason for hiding this comment

vitalybuka commented Oct 4, 2024

goldsteinn commented Oct 4, 2024

vitalybuka commented Oct 4, 2024 • edited Loading

goldsteinn commented Oct 4, 2024

eugenis commented Oct 4, 2024

eugenis commented Oct 4, 2024

eugenis commented Oct 4, 2024

[InstCombine] Folding `(icmp eq/ne (and X, -P2), INT_MIN)` #110880

[InstCombine] Folding `(icmp eq/ne (and X, -P2), INT_MIN)` #110880

goldsteinn commented Oct 2, 2024 •

edited

Loading

vitalybuka commented Oct 4, 2024 •

edited

Loading