Skip to content

Reland "[InstCombine] Fold (sub nuw X, (Y << nuw Z)) >>u exact Z --> (X >>u exact Z) sub nuw Y" #93571

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 3, 2024

Conversation

AZero13
Copy link
Contributor

@AZero13 AZero13 commented May 28, 2024

This is the same fold as ((X << nuw Z) sub nuw Y) >>u exact Z --> X sub nuw (Y >>u exact Z), but with the sub operands swapped.

Alive2 Proof:
https://alive2.llvm.org/ce/z/pT-RxG

@AZero13 AZero13 requested a review from nikic as a code owner May 28, 2024 16:21
@llvmbot
Copy link
Member

llvmbot commented May 28, 2024

@llvm/pr-subscribers-llvm-transforms

Author: AtariDreams (AtariDreams)

Changes

This is the same fold as ((X << nuw Z) sub nuw Y) >>u exact Z --> X sub nuw (Y >>u exact Z), but with the sub operands swapped.

Proof: https://alive2.llvm.org/ce/z/2cRcdx

Relanding #91386 (comment), approved by @dtcxzyw


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp (+11)
  • (modified) llvm/test/Transforms/InstCombine/lshr.ll (+27)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
index ba297111d945f..8d0209ccd13ef 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
@@ -1271,6 +1271,17 @@ Instruction *InstCombinerImpl::visitLShr(BinaryOperator &I) {
     return NewSub;
   }
 
+  // (sub nuw X, (Y << nuw Z)) >>u exact Z --> (X >>u exact Z) sub nuw Y
+  if (I.isExact() &&
+      match(Op0, m_OneUse(m_NUWSub(m_Value(X),
+                                   m_NUWShl(m_Value(Y), m_Specific(Op1)))))) {
+    Value *NewLshr = Builder.CreateLShr(X, Op1, "", /*isExact=*/true);
+    auto *NewSub = BinaryOperator::CreateNUWSub(NewLshr, Y);
+    NewSub->setHasNoSignedWrap(
+        cast<OverflowingBinaryOperator>(Op0)->hasNoSignedWrap());
+    return NewSub;
+  }
+
   auto isSuitableBinOpcode = [](Instruction::BinaryOps BinOpcode) {
     switch (BinOpcode) {
     default:
diff --git a/llvm/test/Transforms/InstCombine/lshr.ll b/llvm/test/Transforms/InstCombine/lshr.ll
index fa92c1c4b3be4..1e370d960c45d 100644
--- a/llvm/test/Transforms/InstCombine/lshr.ll
+++ b/llvm/test/Transforms/InstCombine/lshr.ll
@@ -466,6 +466,33 @@ define i32 @shl_sub_lshr(i32 %x, i32 %c, i32 %y) {
   ret i32 %lshr
 }
 
+define i32 @shl_sub_lshr_reverse(i32 %x, i32 %c, i32 %y) {
+; CHECK-LABEL: @shl_sub_lshr_reverse(
+; CHECK-NEXT:    [[TMP1:%.*]] = lshr exact i32 [[Y:%.*]], [[C:%.*]]
+; CHECK-NEXT:    [[LSHR:%.*]] = sub nuw nsw i32 [[TMP1]], [[X:%.*]]
+; CHECK-NEXT:    ret i32 [[LSHR]]
+;
+  %shl = shl nuw i32 %x, %c
+  %sub = sub nuw nsw i32 %y, %shl
+  %lshr = lshr exact i32 %sub, %c
+  ret i32 %lshr
+}
+
+; Negative test
+
+define i32 @shl_sub_lshr_reverse_no_exact(i32 %x, i32 %c, i32 %y) {
+; CHECK-LABEL: @shl_sub_lshr_reverse_no_exact(
+; CHECK-NEXT:    [[SHL:%.*]] = shl nuw i32 [[X:%.*]], [[C:%.*]]
+; CHECK-NEXT:    [[SUB:%.*]] = sub nuw nsw i32 [[Y:%.*]], [[SHL]]
+; CHECK-NEXT:    [[LSHR:%.*]] = lshr i32 [[SUB]], [[C]]
+; CHECK-NEXT:    ret i32 [[LSHR]]
+;
+  %shl = shl nuw i32 %x, %c
+  %sub = sub nuw nsw i32 %y, %shl
+  %lshr = lshr i32 %sub, %c
+  ret i32 %lshr
+}
+
 define i32 @shl_or_lshr(i32 %x, i32 %c, i32 %y) {
 ; CHECK-LABEL: @shl_or_lshr(
 ; CHECK-NEXT:    [[TMP1:%.*]] = lshr i32 [[Y:%.*]], [[C:%.*]]

@AZero13 AZero13 requested a review from dtcxzyw May 28, 2024 16:32
@AZero13
Copy link
Contributor Author

AZero13 commented May 28, 2024

@dtcxzyw Is this good to be relanded now?

@dtcxzyw
Copy link
Member

dtcxzyw commented May 28, 2024

BTW, don't @ someone in the PR description/commit message, it is annoying.

@AZero13 AZero13 force-pushed the binop-2 branch 2 times, most recently from 9f099ce to b44b4ec Compare May 29, 2024 23:16
@AZero13 AZero13 requested a review from dtcxzyw May 29, 2024 23:17
@AZero13 AZero13 force-pushed the binop-2 branch 4 times, most recently from d7a902f to 721baca Compare May 30, 2024 03:09
AZero13 added 2 commits May 30, 2024 07:31
…xact Z) sub nuw Y

This is the same fold as ((X << nuw Z) sub nuw Y) >>u exact Z --> X sub nuw (Y >>u exact Z), but with the sub operands swapped.

Alive2 Proof:
https://alive2.llvm.org/ce/z/2cRcdx
@AZero13
Copy link
Contributor Author

AZero13 commented May 30, 2024

@dtcxzyw I did all the possible negative tests.

I think this can be merged now!

@dtcxzyw dtcxzyw requested a review from goldsteinn May 30, 2024 13:57
@AZero13
Copy link
Contributor Author

AZero13 commented May 31, 2024

@nikic Ping?

@goldsteinn
Copy link
Contributor

This LGTM, please wait for another approval before pushing.

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.

I'll merge this on Monday if nobody beats me to it.

@nikic nikic merged commit 10e7671 into llvm:main Jun 3, 2024
7 checks passed
@AZero13 AZero13 deleted the binop-2 branch June 3, 2024 09:36
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