[InstCombine] Fix Failure to fold (and %x, (sext i1 %m)) -> (select %m, %x, 0) with multiple uses of %m #81409

SahilPatidar · 2024-02-11T12:08:03Z

github-actions · 2024-02-11T12:08:21Z

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-02-11T12:08:52Z

@llvm/pr-subscribers-llvm-transforms

Author: None (SahilPatidar)

Changes

Resolve #81288

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp (+1-1)
(modified) llvm/test/Transforms/InstCombine/and.ll (+17-2)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index 5fd944a859ef09..516af05b4cc531 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -2637,7 +2637,7 @@ Instruction *InstCombinerImpl::visitAnd(BinaryOperator &I) {
   //       arm may not be reversible due to poison semantics. Is that a good
   //       canonicalization?
   Value *A, *B;
-  if (match(&I, m_c_And(m_OneUse(m_SExt(m_Value(A))), m_Value(B))) &&
+  if (match(&I, m_c_And(m_SExt(m_Value(A)), m_Value(B))) &&
       A->getType()->isIntOrIntVectorTy(1))
     return SelectInst::Create(A, B, Constant::getNullValue(Ty));
 
diff --git a/llvm/test/Transforms/InstCombine/and.ll b/llvm/test/Transforms/InstCombine/and.ll
index 2e37fee07cc47b..0931c7a1c4053d 100644
--- a/llvm/test/Transforms/InstCombine/and.ll
+++ b/llvm/test/Transforms/InstCombine/and.ll
@@ -1818,7 +1818,7 @@ define i8 @not_ashr_bitwidth_mask_use2(i8 %x, i8 %y) {
 ; CHECK-NEXT:    [[ISNOTNEG:%.*]] = icmp sgt i8 [[X:%.*]], -1
 ; CHECK-NEXT:    [[NOT:%.*]] = sext i1 [[ISNOTNEG]] to i8
 ; CHECK-NEXT:    call void @use8(i8 [[NOT]])
-; CHECK-NEXT:    [[R:%.*]] = and i8 [[NOT]], [[Y:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = select i1 [[ISNOTNEG]], i8 [[Y:%.*]], i8 0
 ; CHECK-NEXT:    ret i8 [[R]]
 ;
   %sign = ashr i8 %x, 7
@@ -1925,7 +1925,7 @@ define i16 @invert_signbit_splat_mask_use3(i8 %x, i16 %y) {
 ; CHECK-NEXT:    [[ISNOTNEG:%.*]] = icmp sgt i8 [[X:%.*]], -1
 ; CHECK-NEXT:    [[S:%.*]] = sext i1 [[ISNOTNEG]] to i16
 ; CHECK-NEXT:    call void @use16(i16 [[S]])
-; CHECK-NEXT:    [[R:%.*]] = and i16 [[S]], [[Y:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = select i1 [[ISNOTNEG]], i16 [[Y:%.*]], i16 0
 ; CHECK-NEXT:    ret i16 [[R]]
 ;
   %a = ashr i8 %x, 7
@@ -2768,3 +2768,18 @@ define i32 @add_constant_equal_with_the_top_bit_of_demandedbits_insertpt(i32 %x,
   %and = and i32 %or, 24
   ret i32 %and
 }
+
+define i32 @and_sext(i32 %x, i32 %y, i32 %a, i32 %b) {
+; CHECK-LABEL: @and_sext(
+; CHECK-NEXT:    [[CMP:%.*]] = icmp sgt i32 [[X:%.*]], [[Y:%.*]]
+; CHECK-NEXT:    [[TMP1:%.*]] = add i32 [[A:%.*]], [[B:%.*]]
+; CHECK-NEXT:    [[ADD:%.*]] = select i1 [[CMP]], i32 [[TMP1]], i32 0
+; CHECK-NEXT:    ret i32 [[ADD]]
+;
+  %cmp = icmp sgt i32 %x, %y
+  %sext = sext i1 %cmp to i32
+  %and1 = and i32 %sext, %a
+  %and2 = and i32 %sext, %b
+  %add = add i32 %and1, %and2
+  ret i32 %add
+}

RKSimon · 2024-02-11T12:11:41Z

llvm/test/Transforms/InstCombine/and.ll

+  %and2 = and i32 %sext, %b
+  %add = add i32 %and1, %and2
+  ret i32 %add
+}


You need a better name - and_sext_multiuse at the very least. You also need to add some variants of the test with commuted operands / different types of use of the sext - maybe see what happens if you temporarily disable the and(sext(A), B) fold entirely and use the update script to see what other tests are dependent on this fold and what form they take.

TBH, I don't think we need anything beyond this test. We already have all the necessary commuted / vector / multi-use coverage above. This test essentially just serves to illustrate why relaxing the one-use restriction is useful.

PR Link: llvm/llvm-project#81409

… multiple uses of %m Resolve llvm#81288

RKSimon · 2024-02-15T10:40:05Z

@nikic are you happy with the test coverage here?

llvm/test/Transforms/InstCombine/and.ll

nikic · 2024-02-15T11:39:40Z

llvm/test/Transforms/InstCombine/and.ll

+  %and2 = and i32 %sext, %b
+  %add = add i32 %and1, %and2
+  ret i32 %add
+}


TBH, I don't think we need anything beyond this test. We already have all the necessary commuted / vector / multi-use coverage above. This test essentially just serves to illustrate why relaxing the one-use restriction is useful.

llvm/test/Transforms/InstCombine/and.ll

nikic

LGTM

nikic · 2024-02-16T11:40:43Z

Actually, it looks like there are two legitimate test failures:

Failed Tests (2):
  LLVM :: Transforms/InstCombine/binop-cast.ll
  LLVM :: Transforms/InstCombine/sub-ashr-and-to-icmp-select.ll

Can you please update these tests as well?

SahilPatidar · 2024-02-16T15:40:57Z

ok

SahilPatidar · 2024-02-16T16:11:30Z

Please let me know if any changes need to be made.

goldsteinn · 2024-02-16T16:46:26Z

llvm/test/Transforms/InstCombine/binop-cast.ll

@@ -50,7 +50,7 @@ define i32 @and_sext_to_sel_multi_use(i32 %x, i1 %y) {
 ; CHECK-LABEL: @and_sext_to_sel_multi_use(
 ; CHECK-NEXT:    [[SEXT:%.*]] = sext i1 [[Y:%.*]] to i32
 ; CHECK-NEXT:    call void @use(i32 [[SEXT]])
-; CHECK-NEXT:    [[R:%.*]] = and i32 [[SEXT]], [[X:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = select i1 [[Y]], i32 [[X:%.*]], i32 0


Think this one could classified as regression but probably not a big deal.

nikic

LGTM

github-actions · 2024-02-19T10:07:32Z

@SahilPatidar 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 recieve 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!

SahilPatidar requested a review from nikic as a code owner February 11, 2024 12:08

llvmbot added the llvm:transforms label Feb 11, 2024

RKSimon reviewed Feb 11, 2024

View reviewed changes

dtcxzyw requested a review from goldsteinn February 11, 2024 12:15

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Feb 11, 2024

pre-commit: test PR81409

bd4e569

PR Link: llvm/llvm-project#81409

dtcxzyw mentioned this pull request Feb 11, 2024

pre-commit: test PR81409 dtcxzyw/llvm-opt-benchmark#218

Open

Fix Failure to fold (and %x, (sext i1 %m)) -> (select %m, %x, 0) with…

c745528

… multiple uses of %m Resolve llvm#81288

SahilPatidar force-pushed the instCombine_resolve branch from d5da9cf to c745528 Compare February 11, 2024 17:29

SahilPatidar requested a review from RKSimon February 15, 2024 06:58

nikic reviewed Feb 15, 2024

View reviewed changes

Some fixes in test coverage

e52f1c0

nikic approved these changes Feb 16, 2024

View reviewed changes

nikic changed the title ~~Fix Failure to fold (and %x, (sext i1 %m)) -> (select %m, %x, 0) with multiple uses of %m Resolve #81288~~ [InstCombine] Fix Failure to fold (and %x, (sext i1 %m)) -> (select %m, %x, 0) with multiple uses of %m Feb 16, 2024

update tests

70e3b08

goldsteinn reviewed Feb 16, 2024

View reviewed changes

SahilPatidar requested a review from nikic February 19, 2024 06:44

nikic approved these changes Feb 19, 2024

View reviewed changes

nikic merged commit 4b483ec into llvm:main Feb 19, 2024
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[InstCombine] Fix Failure to fold (and %x, (sext i1 %m)) -> (select %m, %x, 0) with multiple uses of %m #81409

[InstCombine] Fix Failure to fold (and %x, (sext i1 %m)) -> (select %m, %x, 0) with multiple uses of %m #81409

SahilPatidar commented Feb 11, 2024

github-actions bot commented Feb 11, 2024

llvmbot commented Feb 11, 2024

RKSimon Feb 11, 2024

nikic Feb 15, 2024

RKSimon commented Feb 15, 2024

nikic Feb 15, 2024

nikic left a comment

nikic commented Feb 16, 2024

SahilPatidar commented Feb 16, 2024

SahilPatidar commented Feb 16, 2024

goldsteinn Feb 16, 2024

nikic left a comment

github-actions bot commented Feb 19, 2024

[InstCombine] Fix Failure to fold (and %x, (sext i1 %m)) -> (select %m, %x, 0) with multiple uses of %m #81409

[InstCombine] Fix Failure to fold (and %x, (sext i1 %m)) -> (select %m, %x, 0) with multiple uses of %m #81409

Conversation

SahilPatidar commented Feb 11, 2024

github-actions bot commented Feb 11, 2024

llvmbot commented Feb 11, 2024

RKSimon Feb 11, 2024

Choose a reason for hiding this comment

nikic Feb 15, 2024

Choose a reason for hiding this comment

RKSimon commented Feb 15, 2024

nikic Feb 15, 2024

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

nikic commented Feb 16, 2024

SahilPatidar commented Feb 16, 2024

SahilPatidar commented Feb 16, 2024

goldsteinn Feb 16, 2024

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 19, 2024