Skip to content
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

[InstCombine] When -A + B both have nsw flag, set nsw flag. #72127

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

Z572
Copy link
Contributor

@Z572 Z572 commented Nov 13, 2023

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 13, 2023

@llvm/pr-subscribers-llvm-transforms

Author: None (Z572)

Changes

Fixes #72119

https://alive2.llvm.org/ce/z/5f_QuC


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

3 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp (+5-1)
  • (modified) llvm/test/Transforms/InstCombine/add.ll (+30)
  • (modified) llvm/test/Transforms/InstCombine/pr14365.ll (+2-2)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
index 22fd3edc39acb03..9ff1a57b6be93ba 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
@@ -1487,7 +1487,11 @@ Instruction *InstCombinerImpl::visitAdd(BinaryOperator &I) {
       return BinaryOperator::CreateNeg(Builder.CreateAdd(A, B));
 
     // -A + B --> B - A
-    return BinaryOperator::CreateSub(RHS, A);
+    auto *Sub = BinaryOperator::CreateSub(RHS, A);
+    if (auto *Op1 = dyn_cast<BinaryOperator>(LHS)) {
+      Sub->setHasNoSignedWrap(I.hasNoSignedWrap() && Op1->hasNoSignedWrap());
+    }
+    return Sub;
   }
 
   // A + -B  -->  A - B
diff --git a/llvm/test/Transforms/InstCombine/add.ll b/llvm/test/Transforms/InstCombine/add.ll
index 7234fb39e566ebf..fff839c2d89a138 100644
--- a/llvm/test/Transforms/InstCombine/add.ll
+++ b/llvm/test/Transforms/InstCombine/add.ll
@@ -120,6 +120,36 @@ define i32 @test5(i32 %A, i32 %B) {
   ret i32 %D
 }
 
+define i32 @test5_2(i32 %A, i32 %B) {
+; CHECK-LABEL: @test5_2(
+; CHECK-NEXT:    [[D:%.*]] = sub nsw i32 [[B:%.*]], [[A:%.*]]
+; CHECK-NEXT:    ret i32 [[D]]
+;
+  %C = sub nsw i32 0, %A
+  %D = add nsw i32 %C, %B
+  ret i32 %D
+}
+
+define i32 @test5_3(i32 %A, i32 %B) {
+; CHECK-LABEL: @test5_3(
+; CHECK-NEXT:    [[D:%.*]] = sub i32 [[B:%.*]], [[A:%.*]]
+; CHECK-NEXT:    ret i32 [[D]]
+;
+  %C = sub nsw i32 0, %A
+  %D = add i32 %C, %B
+  ret i32 %D
+}
+
+define i32 @test5_4(i32 %A, i32 %B) {
+; CHECK-LABEL: @test5_4(
+; CHECK-NEXT:    [[D:%.*]] = sub i32 [[B:%.*]], [[A:%.*]]
+; CHECK-NEXT:    ret i32 [[D]]
+;
+  %C = sub i32 0, %A
+  %D = add nsw i32 %C, %B
+  ret i32 %D
+}
+
 define <2 x i8> @neg_op0_vec_undef_elt(<2 x i8> %a, <2 x i8> %b) {
 ; CHECK-LABEL: @neg_op0_vec_undef_elt(
 ; CHECK-NEXT:    [[R:%.*]] = sub <2 x i8> [[B:%.*]], [[A:%.*]]
diff --git a/llvm/test/Transforms/InstCombine/pr14365.ll b/llvm/test/Transforms/InstCombine/pr14365.ll
index 5e8dca13fa1b4d8..3a09b55aba309d9 100644
--- a/llvm/test/Transforms/InstCombine/pr14365.ll
+++ b/llvm/test/Transforms/InstCombine/pr14365.ll
@@ -31,7 +31,7 @@ define i32 @test1(i32 %a0) {
 ; CHECK-LABEL: @test1(
 ; CHECK-NEXT:    [[TMP1:%.*]] = lshr i32 [[A0:%.*]], 1
 ; CHECK-NEXT:    [[TMP2:%.*]] = and i32 [[TMP1]], 1431655765
-; CHECK-NEXT:    [[TMP3:%.*]] = sub i32 [[A0]], [[TMP2]]
+; CHECK-NEXT:    [[TMP3:%.*]] = sub nsw i32 [[A0]], [[TMP2]]
 ; CHECK-NEXT:    ret i32 [[TMP3]]
 ;
   %1 = ashr i32 %a0, 1
@@ -46,7 +46,7 @@ define <4 x i32> @test1_vec(<4 x i32> %a0) {
 ; CHECK-LABEL: @test1_vec(
 ; CHECK-NEXT:    [[TMP1:%.*]] = lshr <4 x i32> [[A0:%.*]], <i32 1, i32 1, i32 1, i32 1>
 ; CHECK-NEXT:    [[TMP2:%.*]] = and <4 x i32> [[TMP1]], <i32 1431655765, i32 1431655765, i32 1431655765, i32 1431655765>
-; CHECK-NEXT:    [[TMP3:%.*]] = sub <4 x i32> [[A0]], [[TMP2]]
+; CHECK-NEXT:    [[TMP3:%.*]] = sub nsw <4 x i32> [[A0]], [[TMP2]]
 ; CHECK-NEXT:    ret <4 x i32> [[TMP3]]
 ;
   %1 = ashr <4 x i32> %a0, <i32 1, i32 1, i32 1, i32 1>

Comment on lines 1491 to 1493
if (auto *Op1 = dyn_cast<BinaryOperator>(LHS)) {
Sub->setHasNoSignedWrap(I.hasNoSignedWrap() && Op1->hasNoSignedWrap());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (auto *Op1 = dyn_cast<BinaryOperator>(LHS)) {
Sub->setHasNoSignedWrap(I.hasNoSignedWrap() && Op1->hasNoSignedWrap());
}
auto *OBO = cast<OverflowingBinaryOperator>(LHS));
Sub->setHasNoSignedWrap(I.hasNoSignedWrap() && OBO->hasNoSignedWrap());

Copy link
Member

Choose a reason for hiding this comment

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

This was not fixed in the committed version, it's still using dyn_cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

llvm/test/Transforms/InstCombine/add.ll Outdated Show resolved Hide resolved
llvm/test/Transforms/InstCombine/add.ll Outdated Show resolved Hide resolved
llvm/test/Transforms/InstCombine/add.ll Outdated Show resolved Hide resolved
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

@Z572 Z572 merged commit 3c037b7 into llvm:main Nov 14, 2023
3 checks passed
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.

[InstCombine] Clang failed to fold (-a + b) / (a - b) into -1 due to the lack of nsw flag.
5 participants