Skip to content

[InstCombine] Relax one-use requirement for add iN (sext i1 X), (sext i1 Y) --> sext (X | Y) to iN #90509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

AZero13
Copy link
Contributor

@AZero13 AZero13 commented Apr 29, 2024

Since these remove instructions as long as at least one of X or Y is one-use, we don't need to check one-use for both.

@llvmbot
Copy link
Member

llvmbot commented Apr 29, 2024

@llvm/pr-subscribers-llvm-transforms

Author: AtariDreams (AtariDreams)

Changes

Since these remove instructions, we can forgo the one-use check.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp (+1-3)
  • (modified) llvm/test/Transforms/InstCombine/add.ll (+11-17)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
index 6739b8745d74e4..953b0da564a48f 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
@@ -510,9 +510,7 @@ Value *InstCombinerImpl::SimplifyDemandedUseBits(Value *V, APInt DemandedMask,
       }
 
       // add iN (sext i1 X), (sext i1 Y) --> sext (X | Y) to iN
-      // TODO: Relax the one-use checks because we are removing an instruction?
-      if (match(I, m_Add(m_OneUse(m_SExt(m_Value(X))),
-                         m_OneUse(m_SExt(m_Value(Y))))) &&
+      if (match(I, m_Add(m_SExt(m_Value(X)), m_SExt(m_Value(Y)))) &&
           X->getType()->isIntOrIntVectorTy(1) && X->getType() == Y->getType()) {
         // Truth table for inputs and output signbits:
         //       X:0 | X:1
diff --git a/llvm/test/Transforms/InstCombine/add.ll b/llvm/test/Transforms/InstCombine/add.ll
index 56ee54d351e762..5c7826f33074c1 100644
--- a/llvm/test/Transforms/InstCombine/add.ll
+++ b/llvm/test/Transforms/InstCombine/add.ll
@@ -1435,15 +1435,12 @@ define i32 @and31_add_sexts(i1 %x, i1 %y) {
   ret i32 %r
 }
 
-; Negative test - extra use
-
 define i32 @lshr_add_use_sexts(i1 %x, i1 %y, ptr %p) {
 ; CHECK-LABEL: @lshr_add_use_sexts(
 ; CHECK-NEXT:    [[XS:%.*]] = sext i1 [[X:%.*]] to i32
 ; CHECK-NEXT:    store i32 [[XS]], ptr [[P:%.*]], align 4
-; CHECK-NEXT:    [[YS:%.*]] = sext i1 [[Y:%.*]] to i32
-; CHECK-NEXT:    [[SUB:%.*]] = add nsw i32 [[XS]], [[YS]]
-; CHECK-NEXT:    [[R:%.*]] = lshr i32 [[SUB]], 31
+; CHECK-NEXT:    [[TMP1:%.*]] = or i1 [[X]], [[Y:%.*]]
+; CHECK-NEXT:    [[R:%.*]] = zext i1 [[TMP1]] to i32
 ; CHECK-NEXT:    ret i32 [[R]]
 ;
   %xs = sext i1 %x to i32
@@ -1454,15 +1451,12 @@ define i32 @lshr_add_use_sexts(i1 %x, i1 %y, ptr %p) {
   ret i32 %r
 }
 
-; Negative test - extra use
-
 define i32 @lshr_add_use2_sexts(i1 %x, i1 %y, ptr %p) {
 ; CHECK-LABEL: @lshr_add_use2_sexts(
-; CHECK-NEXT:    [[XS:%.*]] = sext i1 [[X:%.*]] to i32
 ; CHECK-NEXT:    [[YS:%.*]] = sext i1 [[Y:%.*]] to i32
 ; CHECK-NEXT:    store i32 [[YS]], ptr [[P:%.*]], align 4
-; CHECK-NEXT:    [[SUB:%.*]] = add nsw i32 [[XS]], [[YS]]
-; CHECK-NEXT:    [[R:%.*]] = lshr i32 [[SUB]], 31
+; CHECK-NEXT:    [[TMP1:%.*]] = or i1 [[X:%.*]], [[Y]]
+; CHECK-NEXT:    [[R:%.*]] = zext i1 [[TMP1]] to i32
 ; CHECK-NEXT:    ret i32 [[R]]
 ;
   %xs = sext i1 %x to i32
@@ -4018,8 +4012,8 @@ define i32 @add_reduce_sqr_sum_varC_invalid2(i32 %a, i32 %b) {
 
 define i32 @fold_sext_addition_or_disjoint(i8 %x) {
 ; CHECK-LABEL: @fold_sext_addition_or_disjoint(
-; CHECK-NEXT:    [[SE:%.*]] = sext i8 [[XX:%.*]] to i32
-; CHECK-NEXT:    [[R:%.*]] = add nsw i32 [[SE]], 1246
+; CHECK-NEXT:    [[TMP1:%.*]] = sext i8 [[X:%.*]] to i32
+; CHECK-NEXT:    [[R:%.*]] = add nsw i32 [[TMP1]], 1246
 ; CHECK-NEXT:    ret i32 [[R]]
 ;
   %xx = or disjoint i8 %x, 12
@@ -4043,8 +4037,8 @@ define i32 @fold_sext_addition_fail(i8 %x) {
 
 define i32 @fold_zext_addition_or_disjoint(i8 %x) {
 ; CHECK-LABEL: @fold_zext_addition_or_disjoint(
-; CHECK-NEXT:    [[SE:%.*]] = zext i8 [[XX:%.*]] to i32
-; CHECK-NEXT:    [[R:%.*]] = add nuw nsw i32 [[SE]], 1246
+; CHECK-NEXT:    [[TMP1:%.*]] = zext i8 [[X:%.*]] to i32
+; CHECK-NEXT:    [[R:%.*]] = add nuw nsw i32 [[TMP1]], 1246
 ; CHECK-NEXT:    ret i32 [[R]]
 ;
   %xx = or disjoint i8 %x, 12
@@ -4055,9 +4049,9 @@ define i32 @fold_zext_addition_or_disjoint(i8 %x) {
 
 define i32 @fold_zext_addition_or_disjoint2(i8 %x) {
 ; CHECK-LABEL: @fold_zext_addition_or_disjoint2(
-; CHECK-NEXT:    [[XX:%.*]] = add nuw i8 [[X:%.*]], 4
-; CHECK-NEXT:    [[SE:%.*]] = zext i8 [[XX]] to i32
-; CHECK-NEXT:    ret i32 [[SE]]
+; CHECK-NEXT:    [[TMP1:%.*]] = add nuw i8 [[X:%.*]], 4
+; CHECK-NEXT:    [[R:%.*]] = zext i8 [[TMP1]] to i32
+; CHECK-NEXT:    ret i32 [[R]]
 ;
   %xx = or disjoint i8 %x, 18
   %se = zext i8 %xx to i32

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Increases instruction count if both sexts have extra uses.

@AZero13
Copy link
Contributor Author

AZero13 commented Apr 30, 2024

I guess I'm gonna test that then

@AZero13
Copy link
Contributor Author

AZero13 commented Apr 30, 2024

Increases instruction count if both sexts have extra uses.

Just checked and at worst the count stays the same.

@topperc
Copy link
Collaborator

topperc commented Apr 30, 2024

Increases instruction count if both sexts have extra uses.

Just checked and at worst the count stays the same.

In the worst case doesn't it remove 1 Add and insert an Or and a SExt. That's an increase of 1.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case with two extra uses is still untested?

@AZero13 AZero13 force-pushed the one-use branch 2 times, most recently from 831777e to de4321e Compare April 30, 2024 05:05
@AZero13 AZero13 force-pushed the one-use branch 2 times, most recently from 43a21ae to 0c6cfb3 Compare May 2, 2024 18:11
AZero13 added 2 commits June 25, 2024 23:36
…t i1 Y) --> sext (X | Y) to iN

Since these remove instructions, we can forgo the one-use check, as long as at least ONE of the two sexts are one-use.
@goldsteinn
Copy link
Contributor

The the title should the "Relax one-use..." instead of "Remove one-use..."

@AZero13 AZero13 changed the title [InstCombine] Remove one-use requirement for add iN (sext i1 X), (sext i1 Y) --> sext (X | Y) to iN [InstCombine] Relax one-use requirement for add iN (sext i1 X), (sext i1 Y) --> sext (X | Y) to iN Jun 28, 2024
@AZero13
Copy link
Contributor Author

AZero13 commented Jun 28, 2024

The the title should the "Relax one-use..." instead of "Remove one-use..."

Fixed!

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nikic nikic merged commit dd0245e into llvm:main Jun 28, 2024
7 checks passed
@goldsteinn
Copy link
Contributor

@nikic
Copy link
Contributor

nikic commented Jun 28, 2024

@goldsteinn That range also contains other commits. The regression is mostly #96851.

@AZero13 AZero13 deleted the one-use branch June 28, 2024 14:00
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
… i1 Y) --> sext (X | Y) to iN (llvm#90509)

Since these remove instructions as long as at least one of X or Y is
one-use, we don't need to check one-use for both.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants