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] Don't transform sub X, ~Y -> add X, -Y unless Y is actually negatable #72767

Closed

Conversation

goldsteinn
Copy link
Contributor

  • [InstCombine] Add tests for improving sub X, ~Y -> add X, -Y; NFC
  • [InstCombine] Don't transform sub X, ~Y -> add X, -Y unless Y is actually negatable

…s actually negatable

This combine was previously adding instruction in some cases (see the
tests).
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 18, 2023

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: None (goldsteinn)

Changes
  • [InstCombine] Add tests for improving sub X, ~Y -> add X, -Y; NFC
  • [InstCombine] Don't transform sub X, ~Y -> add X, -Y unless Y is actually negatable

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

3 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp (+5-3)
  • (modified) llvm/test/Analysis/ValueTracking/knownbits-and-or-xor-lowbit.ll (+2-3)
  • (modified) llvm/test/Transforms/InstCombine/fold-sub-of-not-to-inc-of-add.ll (+20-7)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp b/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
index 53b34b03ca78070..6e0b44c11f22a45 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp
@@ -447,9 +447,11 @@ std::array<Value *, 2> Negator::getSortedOperandsOfBinOp(Instruction *I) {
     // `xor` is negatible if one of its operands is invertible.
     // FIXME: InstCombineInverter? But how to connect Inverter and Negator?
     if (auto *C = dyn_cast<Constant>(Ops[1])) {
-      Value *Xor = Builder.CreateXor(Ops[0], ConstantExpr::getNot(C));
-      return Builder.CreateAdd(Xor, ConstantInt::get(Xor->getType(), 1),
-                               I->getName() + ".neg");
+      if (IsTrulyNegation) {
+        Value *Xor = Builder.CreateXor(Ops[0], ConstantExpr::getNot(C));
+        return Builder.CreateAdd(Xor, ConstantInt::get(Xor->getType(), 1),
+                                 I->getName() + ".neg");
+      }
     }
     return nullptr;
   }
diff --git a/llvm/test/Analysis/ValueTracking/knownbits-and-or-xor-lowbit.ll b/llvm/test/Analysis/ValueTracking/knownbits-and-or-xor-lowbit.ll
index 7fbc78079c24029..f44948e6a089f88 100644
--- a/llvm/test/Analysis/ValueTracking/knownbits-and-or-xor-lowbit.ll
+++ b/llvm/test/Analysis/ValueTracking/knownbits-and-or-xor-lowbit.ll
@@ -106,9 +106,8 @@ define <2 x i1> @sub_XY_and_bit0_is_zero_fail(<2 x i8> %x, <2 x i8> %C) nounwind
 
 define i1 @sub_XY_xor_bit0_is_one_fail(i8 %x, i8 %C) nounwind {
 ; CHECK-LABEL: @sub_XY_xor_bit0_is_one_fail(
-; CHECK-NEXT:    [[TMP1:%.*]] = xor i8 [[C:%.*]], -2
-; CHECK-NEXT:    [[C1_NEG:%.*]] = add i8 [[TMP1]], 1
-; CHECK-NEXT:    [[Y:%.*]] = add i8 [[C1_NEG]], [[X:%.*]]
+; CHECK-NEXT:    [[C1:%.*]] = xor i8 [[C:%.*]], 1
+; CHECK-NEXT:    [[Y:%.*]] = sub i8 [[X:%.*]], [[C1]]
 ; CHECK-NEXT:    [[W:%.*]] = xor i8 [[Y]], [[X]]
 ; CHECK-NEXT:    [[R:%.*]] = icmp eq i8 [[W]], 10
 ; CHECK-NEXT:    ret i1 [[R]]
diff --git a/llvm/test/Transforms/InstCombine/fold-sub-of-not-to-inc-of-add.ll b/llvm/test/Transforms/InstCombine/fold-sub-of-not-to-inc-of-add.ll
index 1e315696c3110c3..6f311f05fb0176e 100644
--- a/llvm/test/Transforms/InstCombine/fold-sub-of-not-to-inc-of-add.ll
+++ b/llvm/test/Transforms/InstCombine/fold-sub-of-not-to-inc-of-add.ll
@@ -22,6 +22,19 @@ define i32 @p0_scalar(i32 %x, i32 %y) {
   ret i32 %t1
 }
 
+define i8 @p0_scalar_not_truly_negatable(i8 %x, i8 %y) {
+; CHECK-LABEL: @p0_scalar_not_truly_negatable(
+; CHECK-NEXT:    [[XX:%.*]] = xor i8 [[X:%.*]], 123
+; CHECK-NEXT:    [[YY:%.*]] = xor i8 [[Y:%.*]], 45
+; CHECK-NEXT:    [[R:%.*]] = sub i8 [[XX]], [[YY]]
+; CHECK-NEXT:    ret i8 [[R]]
+;
+  %xx = xor i8 %x, 123
+  %yy = xor i8 %y, 45
+  %r = sub i8 %xx, %yy
+  ret i8 %r
+}
+
 ;------------------------------------------------------------------------------;
 ; Vector tests
 ;------------------------------------------------------------------------------;
@@ -79,28 +92,28 @@ define i32 @n4(i32 %x, i32 %y) {
 ; CHECK-NEXT:    ret i32 [[T1]]
 ;
   %t0 = xor i32 %x, -1
-  %t1 = sub i32 %t0, %y ; swapped
+  %t1 = sub i32 %t0, %y  ; swapped
   ret i32 %t1
 }
 
 define i32 @n5_is_not_not(i32 %x, i32 %y) {
 ; CHECK-LABEL: @n5_is_not_not(
-; CHECK-NEXT:    [[T0_NEG:%.*]] = add i32 [[X:%.*]], -2147483647
-; CHECK-NEXT:    [[T1:%.*]] = add i32 [[T0_NEG]], [[Y:%.*]]
+; CHECK-NEXT:    [[T0:%.*]] = xor i32 [[X:%.*]], 2147483647
+; CHECK-NEXT:    [[T1:%.*]] = sub i32 [[Y:%.*]], [[T0]]
 ; CHECK-NEXT:    ret i32 [[T1]]
 ;
-  %t0 = xor i32 %x, 2147483647 ; not -1
+  %t0 = xor i32 %x, 2147483647  ; not -1
   %t1 = sub i32 %y, %t0
   ret i32 %t1
 }
 
 define <2 x i32> @n5_is_not_not_vec_splat(<2 x i32> %x, <2 x i32> %y) {
 ; CHECK-LABEL: @n5_is_not_not_vec_splat(
-; CHECK-NEXT:    [[T0_NEG:%.*]] = add <2 x i32> [[X:%.*]], <i32 -2147483647, i32 -2147483647>
-; CHECK-NEXT:    [[T1:%.*]] = add <2 x i32> [[T0_NEG]], [[Y:%.*]]
+; CHECK-NEXT:    [[T0:%.*]] = xor <2 x i32> [[X:%.*]], <i32 2147483647, i32 2147483647>
+; CHECK-NEXT:    [[T1:%.*]] = sub <2 x i32> [[Y:%.*]], [[T0]]
 ; CHECK-NEXT:    ret <2 x i32> [[T1]]
 ;
-  %t0 = xor <2 x i32> %x, <i32 2147483647, i32 2147483647> ; signmask, but not -1
+  %t0 = xor <2 x i32> %x, <i32 2147483647, i32 2147483647>  ; signmask, but not -1
   %t1 = sub <2 x i32> %y, %t0
   ret <2 x i32> %t1
 }

@goldsteinn goldsteinn changed the title goldsteinn/is truly negatable [InstCombine] Don't transform sub X, ~Y -> add X, -Y unless Y is actually negatable Nov 18, 2023
Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

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

LGTM.

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

sr-tream pushed a commit to sr-tream/llvm-project that referenced this pull request Nov 20, 2023
…s actually negatable

This combine was previously adding instruction in some cases (see the
tests).

Closes llvm#72767
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
…s actually negatable

This combine was previously adding instruction in some cases (see the
tests).

Closes llvm#72767
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.

None yet

4 participants