[InstCombine] Missed optimization: Fold (sext(a) & c1) == c2 to (a & c3) == trunc(c2) #112646

leewei05 · 2024-10-17T01:56:09Z

Fixes #85830
Updated Alive proof https://alive2.llvm.org/ce/z/KnvoP5

Since this is my first PR to LLVM, please let me know where I can improve, thanks!

github-actions · 2024-10-17T01:56:28Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-10-17T01:56:58Z

@llvm/pr-subscribers-llvm-transforms

Author: Lee Wei (leewei05)

Changes

Alive2: https://alive2.llvm.org/ce/z/eHAC-5
Fixes the first code pattern from #85830

The original Alive2 proof from the issue has three code patterns and the second pattern is already being optimized by LLVM.
https://alive2.llvm.org/ce/z/44YNAL

Since this is my first PR to LLVM, I was thinking that I can add a follow-up PR for the third pattern after merging this one. Please let me know where I can improve, thanks!

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp (+24)
(added) llvm/test/Transforms/InstCombine/sext-and.ll (+49)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index 18a6fdcec1728e..8a79c44777c1a4 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -24,6 +24,7 @@
 #include "llvm/IR/ConstantRange.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/InstrTypes.h"
+#include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/PatternMatch.h"
 #include "llvm/Support/KnownBits.h"
@@ -7687,6 +7688,29 @@ Instruction *InstCombinerImpl::visitICmpInst(ICmpInst &I) {
   if (Instruction *Res = foldReductionIdiom(I, Builder, DL))
     return Res;
 
+  {
+    Value *A, *B;
+    const APInt *C;
+    ICmpInst::Predicate PredEq = ICmpInst::ICMP_EQ;
+    if (I.getPredicate() == PredEq) {
+      if (match(Op0, m_And(m_Value(A), m_APInt(C))) && match(Op1, m_One())) {
+        if (*C == APInt::getSignedMinValue(C->getBitWidth()) + 1) {
+          if (match(A, m_SExt(m_Value(B))) &&
+              B->getType()->getIntegerBitWidth() > 2) {
+            auto *InputTy = B->getType();
+            Value *AndInst = Builder.CreateAnd(
+                B,
+                ConstantInt::get(InputTy, APInt::getSignedMinValue(
+                                              InputTy->getIntegerBitWidth()) +
+                                              1));
+            return CmpInst::Create(Instruction::ICmp, PredEq, AndInst,
+                                   ConstantInt::get(InputTy, 1));
+          }
+        }
+      }
+    }
+  }
+
   return Changed ? &I : nullptr;
 }
 
diff --git a/llvm/test/Transforms/InstCombine/sext-and.ll b/llvm/test/Transforms/InstCombine/sext-and.ll
new file mode 100644
index 00000000000000..a4ce0f41a5afb8
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/sext-and.ll
@@ -0,0 +1,49 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+
+declare void @use(i8)
+
+define i1 @fold_sext_to_and(i8 %x) {
+; CHECK-LABEL: define i1 @fold_sext_to_and(
+; CHECK-SAME: i8 [[X:%.*]]) {
+; CHECK-NEXT:    [[TMP1:%.*]] = and i8 [[X]], -127
+; CHECK-NEXT:    [[TMP3:%.*]] = icmp eq i8 [[TMP1]], 1
+; CHECK-NEXT:    ret i1 [[TMP3]]
+;
+  %1 = sext i8 %x to i32
+  %2 = and i32 %1, -2147483647
+  %3 = icmp eq i32 %2, 1
+  ret i1 %3
+}
+
+define i1 @fold_sext_to_and_multi_use(i8 %x) {
+; CHECK-LABEL: define i1 @fold_sext_to_and_multi_use(
+; CHECK-SAME: i8 [[X:%.*]]) {
+; CHECK-NEXT:    [[TMP1:%.*]] = sext i8 [[X]] to i32
+; CHECK-NEXT:    call void @use(i32 [[TMP1]])
+; CHECK-NEXT:    [[TMP2:%.*]] = and i8 [[X]], -127
+; CHECK-NEXT:    [[TMP3:%.*]] = icmp eq i8 [[TMP2]], 1
+; CHECK-NEXT:    ret i1 [[TMP3]]
+;
+  %1 = sext i8 %x to i32
+  call void @use(i32 %1)
+  %2 = and i32 %1, -2147483647
+  %3 = icmp eq i32 %2, 1
+  ret i1 %3
+}
+
+; Negative tests
+
+define i1 @fold_sext_to_and_wrong(i2 %x) {
+; CHECK-LABEL: define i1 @fold_sext_to_and_wrong(
+; CHECK-SAME: i2 [[X:%.*]]) {
+; CHECK-NEXT:    [[TMP1:%.*]] = sext i2 [[X]] to i32
+; CHECK-NEXT:    [[TMP2:%.*]] = and i32 [[TMP1]], -2147483647
+; CHECK-NEXT:    [[TMP3:%.*]] = icmp eq i32 [[TMP2]], 1
+; CHECK-NEXT:    ret i1 [[TMP3]]
+;
+  %1 = sext i2 %x to i32
+  %2 = and i32 %1, -2147483647
+  %3 = icmp eq i32 %2, 1
+  ret i1 %3
+}

nikic

The pattern you implemented looks way too specific to me. You are checking for specific constants C1 and C2, but this should be implemented for any C1/C2 that satisfy the necessary preconditions from the proof.

goldsteinn · 2024-10-17T14:46:55Z

Welcome and thank you for the contribution!

To expand on how to make it generic, see: https://alive2.llvm.org/ce/z/RWYisS

So for your checks on what are valid for C1/C2, you can use {C1,C2}->getNumSignBits() <= A->getType()->getScalarSizeInBits()

goldsteinn · 2024-10-17T14:49:06Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

+    const APInt *C;
+    ICmpInst::Predicate PredEq = ICmpInst::ICMP_EQ;
+    if (I.getPredicate() == PredEq) {
+      if (match(Op0, m_And(m_Value(A), m_APInt(C))) && match(Op1, m_One())) {


Just matching m_And(m_SExtLike(m_Value(A)...) seems simpler.

Note m_SExtLike just matches sext or a zext that is provably equivilent to a sext due to the nneg IR flag.

leewei05 · 2024-10-17T18:10:10Z

To expand on how to make it generic, see: https://alive2.llvm.org/ce/z/RWYisS

@goldsteinn I still cannot understand how to make my implementation more generic from your Alive2 example. Can you shed some light on this? Is there any existing code that you can reference me to? Thank you!

goldsteinn · 2024-10-17T20:15:18Z

To expand on how to make it generic, see: https://alive2.llvm.org/ce/z/RWYisS

@goldsteinn I still cannot understand how to make my implementation more generic from your Alive2 example. Can you shed some light on this? Is there any existing code that you can reference me to? Thank you!

The *C == APInt::getSignedMinValue(C->getBitWidth()) + 1 and match(Op1, m_One()) basically imply that your c1 and c2 can only be those two values respectively.

Instead you can do match(Op1, m_APInt(C2)) then check its okay with C->getNumSignBits() <= A->getType()->getScalarSizeInBits() && C2->getNumSignBits() <= A->getType()->getScalarSizeInBits() which is all the constraint that the proof needs.

leewei05 · 2024-10-18T03:59:29Z

Instead you can do match(Op1, m_APInt(C2)) then check its okay with C->getNumSignBits() <= A->getType()->getScalarSizeInBits() && C2->getNumSignBits() <= A->getType()->getScalarSizeInBits() which is all the constraint that the proof needs.

@goldsteinn
Thanks for the pointers! I tried your approach but I think the approach is a bit different from your example. Example: https://alive2.llvm.org/ce/z/GPZPJD. I can see that C1->getNumSignBits() relates to %b_signbits = select i1 %b_neg, i32 %b_cl1, i32 %b_clz. However, I also see that there is a %b_bits = sub i32 33, %b_signbits before comparing with %a's bit width. Is this step also required to make it work?

Also, I printed the bit value for the following example.

%conv = sext i8 %a to i32
%0 = and i32 %conv, -2147483647
%cmp = icmp eq i32 %0, 1
ret i1 %cmp

Compare directly with C->getNumSignBits() <= A->getType()->getScalarSizeInBits() && C2->getNumSignBits() <= A->getType()->getScalarSizeInBits() couldn't match the above example. Is there a step I missed?

 if (match(Op0, m_And(m_SExtLike(m_Value(A)), m_APInt(C1))) && match(Op1, m_APInt(C2))) {
        auto *InputTy = A->getType();
        errs() << "A: " << InputTy->getScalarSizeInBits() << "\n";
        errs() << "C1: " << C1->getNumSignBits() << "\n";
        errs() << "C2: " << C2->getNumSignBits() << "\n";
        ...
}

A: 8
C1: 1 (-2147483647 = 0x80000001) count leading 1s get 1
C2: 31 (1 = 0x00000001) count leading 0s get 31

leewei05 · 2024-10-25T03:20:04Z

I've clarified the matching pattern of this problem with @XChy and updated the changes based on the providing proof. Thanks again!

Here's an updated Alive2 proof: https://alive2.llvm.org/ce/z/iQUQfE
Examples for Alive2 proof: https://alive2.llvm.org/ce/z/44YNAL

XChy · 2024-10-25T11:25:11Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

+      // c2 must be positive
+      // sext(a) & c1 == c2 --> a & c3 == trunc(c2)
+      if (match(Op0, m_And(m_SExtLike(m_Value(A)), m_APInt(C1))) && match(Op1, m_APInt(C2))) {
+        if (C2->isNonNegative()) {


c2 must be non-negative at the bitwidth of a. For example, if a is an 8-bit integer and c2 is an 32-bit integer, the leading 25 bits of c2 should be zeros.
Here you are actually checking if c2 is non-negative at the bitwidth of c2.

Or just C2.ult(APInt::getOneBitSet(C2.getBitWidth(), InputBitWidth)

XChy · 2024-10-25T11:29:38Z

llvm/test/Transforms/InstCombine/sext-and.ll

+  %2 = and i32 %1, -2147483647
+  %3 = icmp eq i32 %2, -1
+  ret i1 %3
+}


Please add some negative tests, such as:

c2 = 0x80

c2 = 0x80000000

c1 = 0x80

c1 = 0xffffff00

XChy · 2024-10-25T11:30:42Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

+  {
+    Value *A;
+    const APInt *C1, *C2;
+    ICmpInst::Predicate PredEq = ICmpInst::ICMP_EQ;


What about icmp ne cases? See https://alive2.llvm.org/ce/z/MUHREP

XChy · 2024-10-25T11:34:45Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

+          if (Leading1sCount > 0) {
+            TruncC1.setBit(TargetBitSize - 1);
+          }


See https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

Suggested change

if (Leading1sCount > 0) {

TruncC1.setBit(TargetBitSize - 1);

}

if (Leading1sCount > 0)

TruncC1.setBit(TargetBitSize - 1);

goldsteinn · 2024-10-25T14:46:46Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

+          Type *InputTy = A->getType();
+          unsigned TargetBitSize = InputTy->getScalarSizeInBits();
+          // Count the number of 1s in C1 high bits of size TargetBitSize.
+          unsigned Leading1sCount = C1->lshr(C1->getBitWidth() - TargetBitSize).popcount();


Can Leading1sCount just be replaced with !C1->lshr(C1->getBitWidth() - TargetBitSize).isZero()

Or it might be easier to just do C1->uge(APInt::getOneBitWidth(C1->getBitWidth(), InputBitWdith)

goldsteinn · 2024-10-25T14:47:34Z

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

+      if (match(Op0, m_And(m_SExtLike(m_Value(A)), m_APInt(C1))) && match(Op1, m_APInt(C2))) {
+        if (C2->isNonNegative()) {
+          Type *InputTy = A->getType();
+          unsigned TargetBitSize = InputTy->getScalarSizeInBits();


nit: Imo TargetBitSize -> InputBitSize. Target... implies you are refering to the compilation target.

goldsteinn · 2024-10-25T14:58:56Z

Instead you can do match(Op1, m_APInt(C2)) then check its okay with C->getNumSignBits() <= A->getType()->getScalarSizeInBits() && C2->getNumSignBits() <= A->getType()->getScalarSizeInBits() which is all the constraint that the proof needs.

@goldsteinn Thanks for the pointers! I tried your approach but I think the approach is a bit different from your example. Example: https://alive2.llvm.org/ce/z/GPZPJD. I can see that C1->getNumSignBits() relates to %b_signbits = select i1 %b_neg, i32 %b_cl1, i32 %b_clz. However, I also see that there is a %b_bits = sub i32 33, %b_signbits before comparing with %a's bit width. Is this step also required to make it work?

Sorry for missing this message.

The PR is coming along, think you have the right approach now and after another cycle or so if reviews should be good to merge.

leewei05 · 2024-10-25T18:37:38Z

The PR is coming along, think you have the right approach now and after another cycle or so if reviews should be good to merge.

@XChy @goldsteinn
Thanks for the suggestions! I've updated the PR, please take another look!

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

goldsteinn · 2024-10-26T14:12:45Z

llvm/test/Transforms/InstCombine/sext-and.ll

+  %2 = and i32 %1, -256
+  %3 = icmp ne i32 %2, 1
+  ret i1 %3
+}


Can you use ne for some of the positive tests?

I've added positive tests for ne. Please take another look! Thanks!

leewei05 · 2024-11-03T03:17:09Z

ping @goldsteinn

goldsteinn · 2024-11-03T18:09:52Z

ping @goldsteinn

Need to fixup tests:

Failed Tests (4):
  LLVM :: Transforms/InstCombine/load-cmp.ll
  LLVM :: Transforms/LoopVectorize/X86/float-induction-x86.ll
  LLVM :: Transforms/PhaseOrdering/AArch64/extra-unroll-simplifications.ll
  LLVM :: Transforms/PhaseOrdering/X86/vdiv.ll

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

llvm/test/Transforms/InstCombine/sext-and.ll

goldsteinn

LGTM

XChy

LGTM

github-actions · 2024-11-11T11:52:14Z

@leewei05 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

…m#112646) Fixes llvm#85830. Updated Alive proof: https://alive2.llvm.org/ce/z/KnvoP5

leewei05 requested a review from nikic as a code owner October 17, 2024 01:56

llvmbot added the llvm:transforms label Oct 17, 2024

nikic requested changes Oct 17, 2024

View reviewed changes

goldsteinn reviewed Oct 17, 2024

View reviewed changes

leewei05 force-pushed the sext-fold branch from 7b7d698 to c1ee93b Compare October 25, 2024 03:08

leewei05 mentioned this pull request Oct 25, 2024

[InstCombine] Missed optimization: Fold (sext(a) & sext(c1)) == c2 to (a & c1) == c2 or 0 #85830

Closed

leewei05 changed the title ~~[InstCombine] Missed optimization: Fold (sext(a) & sext(c1)) == c2 to (a & c1) == c2~~ [InstCombine] Missed optimization: Fold (sext(a) & c1) == c2 to (a & c1) == trunc(c2) Oct 25, 2024

leewei05 requested review from nikic and goldsteinn October 25, 2024 03:34

XChy changed the title ~~[InstCombine] Missed optimization: Fold (sext(a) & c1) == c2 to (a & c1) == trunc(c2)~~ [InstCombine] Missed optimization: Fold (sext(a) & c1) == c2 to (a & c3) == trunc(c2) Oct 25, 2024

XChy reviewed Oct 25, 2024

View reviewed changes

goldsteinn reviewed Oct 25, 2024

View reviewed changes

leewei05 force-pushed the sext-fold branch 2 times, most recently from 7f82444 to 3d97acf Compare October 25, 2024 18:36

leewei05 requested review from goldsteinn and XChy October 25, 2024 18:38

goldsteinn reviewed Oct 26, 2024

View reviewed changes

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

goldsteinn reviewed Oct 26, 2024

View reviewed changes

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

goldsteinn reviewed Oct 26, 2024

View reviewed changes

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

goldsteinn reviewed Oct 26, 2024

View reviewed changes

leewei05 force-pushed the sext-fold branch 2 times, most recently from 8e912fc to 9110bbd Compare October 26, 2024 20:15

leewei05 requested a review from goldsteinn October 26, 2024 20:21

leewei05 force-pushed the sext-fold branch from 9110bbd to c20fa0e Compare October 31, 2024 01:02

nikic reviewed Nov 3, 2024

View reviewed changes

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

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

llvm/test/Transforms/InstCombine/sext-and.ll Show resolved Hide resolved

Add pre-commit tests

1eaac25

leewei05 force-pushed the sext-fold branch from c20fa0e to df675a5 Compare November 4, 2024 03:51

Apply transformation

1ddd277

leewei05 force-pushed the sext-fold branch from df675a5 to 1ddd277 Compare November 4, 2024 04:09

leewei05 requested a review from nikic November 4, 2024 06:22

goldsteinn approved these changes Nov 4, 2024

View reviewed changes

XChy mentioned this pull request Nov 5, 2024

Task submission dtcxzyw/llvm-opt-benchmark#1312

Open

dtcxzyw mentioned this pull request Nov 6, 2024

pre-commit: PR112646 dtcxzyw/llvm-opt-benchmark#1633

Closed

XChy approved these changes Nov 9, 2024

View reviewed changes

nikic merged commit 6ad1dd3 into llvm:main Nov 11, 2024
8 checks passed

leewei05 deleted the sext-fold branch November 11, 2024 16:15

Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024

[InstCombine] Fold (sext(a) & c1) == c2 to (a & c3) == trunc(c2) (llv…

8535345

…m#112646) Fixes llvm#85830. Updated Alive proof: https://alive2.llvm.org/ce/z/KnvoP5

[InstCombine] Missed optimization: Fold (sext(a) & c1) == c2 to (a & c3) == trunc(c2) #112646

[InstCombine] Missed optimization: Fold (sext(a) & c1) == c2 to (a & c3) == trunc(c2) #112646

Uh oh!

Conversation

leewei05 commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 17, 2024

Uh oh!

llvmbot commented Oct 17, 2024

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

goldsteinn commented Oct 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leewei05 commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goldsteinn commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leewei05 commented Oct 18, 2024

Uh oh!

leewei05 commented Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goldsteinn commented Oct 25, 2024

Uh oh!

leewei05 commented Oct 25, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leewei05 Oct 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leewei05 commented Nov 3, 2024

Uh oh!

goldsteinn commented Nov 3, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goldsteinn left a comment

Choose a reason for hiding this comment

Uh oh!

XChy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Nov 11, 2024

Uh oh!

Uh oh!

leewei05 commented Oct 17, 2024 •

edited

Loading

leewei05 commented Oct 17, 2024 •

edited

Loading

goldsteinn commented Oct 17, 2024 •

edited

Loading

leewei05 commented Oct 25, 2024 •

edited

Loading

leewei05 Oct 26, 2024 •

edited

Loading