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] Preserve flags for abs(X) * abs(X) and nabs(X) * nabs(X) #88662

Closed
wants to merge 2 commits into from

Conversation

AZero13
Copy link
Contributor

@AZero13 AZero13 commented Apr 14, 2024

@llvmbot
Copy link
Member

llvmbot commented Apr 14, 2024

@llvm/pr-subscribers-llvm-transforms

Author: AtariDreams (AtariDreams)

Changes

Alive2 Proofs:

https://alive2.llvm.org/ce/z/CN9BP-
https://alive2.llvm.org/ce/z/kq9vh6
https://alive2.llvm.org/ce/z/LtVSL5


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp (+13-4)
  • (modified) llvm/test/Transforms/InstCombine/mul.ll (+72-1)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
index 4dc1319f1c437f..ee092add097771 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
@@ -323,11 +323,20 @@ Instruction *InstCombinerImpl::visitMul(BinaryOperator &I) {
   if (Op0 == Op1) {
     Value *X, *Y;
     SelectPatternFlavor SPF = matchSelectPattern(Op0, X, Y).Flavor;
-    if (SPF == SPF_ABS || SPF == SPF_NABS)
-      return BinaryOperator::CreateMul(X, X);
+    if (SPF == SPF_NABS ||
+        match(Op0, m_Neg(m_Intrinsic<Intrinsic::abs>(m_Value(X))))) {
+      if (I.hasNoUnsignedWrap()) {
+        Instruction *NewMul = BinaryOperator::CreateNUWMul(X, X);
+        NewMul->setHasNoSignedWrap(true);
+        return NewMul;
+      }
+      return I.hasNoSignedWrap() ? BinaryOperator::CreateNSWMul(X, X)
+                                 : BinaryOperator::CreateMul(X, X);
+    }
 
-    if (match(Op0, m_Intrinsic<Intrinsic::abs>(m_Value(X))))
-      return BinaryOperator::CreateMul(X, X);
+    if (SPF == SPF_ABS || match(Op0, m_Intrinsic<Intrinsic::abs>(m_Value(X))))
+      return I.hasNoSignedWrap() ? BinaryOperator::CreateNSWMul(X, X)
+                                 : BinaryOperator::CreateMul(X, X);
   }
 
   {
diff --git a/llvm/test/Transforms/InstCombine/mul.ll b/llvm/test/Transforms/InstCombine/mul.ll
index d4a689c60786e0..e478e34ddd4859 100644
--- a/llvm/test/Transforms/InstCombine/mul.ll
+++ b/llvm/test/Transforms/InstCombine/mul.ll
@@ -1605,6 +1605,31 @@ define i32 @combine_mul_nabs_i32(i32 %0) {
   ret i32 %m
 }
 
+define i32 @combine_mul_nabs_i32_unsigned_wrap(i32 %0) {
+; CHECK-LABEL: @combine_mul_nabs_i32_unsigned_wrap(
+; CHECK-NEXT:    [[M:%.*]] = mul nuw nsw i32 [[TMP0:%.*]], [[TMP0]]
+; CHECK-NEXT:    ret i32 [[M]]
+;
+  %c = icmp slt i32 %0, 0
+  %s = sub nsw i32 0, %0
+  %r = select i1 %c, i32 %0, i32 %s
+  %m = mul nuw i32 %r, %r
+  ret i32 %m
+}
+
+define i32 @combine_mul_nabs_i32_signed_wrap(i32 %0) {
+; CHECK-LABEL: @combine_mul_nabs_i32_signed_wrap(
+; CHECK-NEXT:    [[M:%.*]] = mul nsw i32 [[TMP0:%.*]], [[TMP0]]
+; CHECK-NEXT:    ret i32 [[M]]
+;
+  %c = icmp slt i32 %0, 0
+  %s = sub nsw i32 0, %0
+  %r = select i1 %c, i32 %0, i32 %s
+  %m = mul nsw i32 %r, %r
+  ret i32 %m
+}
+
+
 define <4 x i32> @combine_mul_nabs_v4i32(<4 x i32> %0) {
 ; CHECK-LABEL: @combine_mul_nabs_v4i32(
 ; CHECK-NEXT:    [[M:%.*]] = mul <4 x i32> [[TMP0:%.*]], [[TMP0]]
@@ -1617,13 +1642,37 @@ define <4 x i32> @combine_mul_nabs_v4i32(<4 x i32> %0) {
   ret <4 x i32> %m
 }
 
+define <4 x i32> @combine_mul_nabs_v4i32_unsigned_wrap(<4 x i32> %0) {
+; CHECK-LABEL: @combine_mul_nabs_v4i32_unsigned_wrap(
+; CHECK-NEXT:    [[M:%.*]] = mul nuw nsw <4 x i32> [[TMP0:%.*]], [[TMP0]]
+; CHECK-NEXT:    ret <4 x i32> [[M]]
+;
+  %c = icmp slt <4 x i32> %0, zeroinitializer
+  %s = sub nsw <4 x i32> zeroinitializer, %0
+  %r = select <4 x i1> %c, <4 x i32> %0, <4 x i32> %s
+  %m = mul nuw <4 x i32> %r, %r
+  ret <4 x i32> %m
+}
+
+define <4 x i32> @combine_mul_nabs_v4i32_signed_wrap(<4 x i32> %0) {
+; CHECK-LABEL: @combine_mul_nabs_v4i32_signed_wrap(
+; CHECK-NEXT:    [[M:%.*]] = mul nsw <4 x i32> [[TMP0:%.*]], [[TMP0]]
+; CHECK-NEXT:    ret <4 x i32> [[M]]
+;
+  %c = icmp slt <4 x i32> %0, zeroinitializer
+  %s = sub nsw <4 x i32> zeroinitializer, %0
+  %r = select <4 x i1> %c, <4 x i32> %0, <4 x i32> %s
+  %m = mul nsw <4 x i32> %r, %r
+  ret <4 x i32> %m
+}
+
 define i32 @combine_mul_abs_intrin(i32 %x) {
 ; CHECK-LABEL: @combine_mul_abs_intrin(
 ; CHECK-NEXT:    [[MUL:%.*]] = mul i32 [[X:%.*]], [[X]]
 ; CHECK-NEXT:    ret i32 [[MUL]]
 ;
   %abs = call i32 @llvm.abs.i32(i32 %x, i1 false)
-  %mul = mul i32 %abs, %abs
+  %mul = mul nuw i32 %abs, %abs
   ret i32 %mul
 }
 
@@ -1638,6 +1687,28 @@ define i32 @combine_mul_nabs_intrin(i32 %x) {
   ret i32 %mul
 }
 
+define i32 @combine_mul_nabs_intrin_unsigned_flags(i32 %x) {
+; CHECK-LABEL: @combine_mul_nabs_intrin_unsigned_flags(
+; CHECK-NEXT:    [[MUL:%.*]] = mul nuw nsw i32 [[X:%.*]], [[X]]
+; CHECK-NEXT:    ret i32 [[MUL]]
+;
+  %abs = call i32 @llvm.abs.i32(i32 %x, i1 false)
+  %neg = sub i32 0, %abs
+  %mul = mul nuw i32 %neg, %neg
+  ret i32 %mul
+}
+
+define i32 @combine_mul_nabs_intrin_signed_flags(i32 %x) {
+; CHECK-LABEL: @combine_mul_nabs_intrin_signed_flags(
+; CHECK-NEXT:    [[MUL:%.*]] = mul nsw i32 [[X:%.*]], [[X]]
+; CHECK-NEXT:    ret i32 [[MUL]]
+;
+  %abs = call i32 @llvm.abs.i32(i32 %x, i1 false)
+  %neg = sub i32 0, %abs
+  %mul = mul nsw i32 %neg, %neg
+  ret i32 %mul
+}
+
 ; z * splat(0) = splat(0), even for scalable vectors
 define <vscale x 2 x i64> @mul_scalable_splat_zero(<vscale x 2 x i64> %z) {
 ; CHECK-LABEL: @mul_scalable_splat_zero(

@AZero13 AZero13 changed the title [InstCombine] abs(X) * abs(X) and nabs(X) * nabs(X) should preserve flags [InstCombine] Preserve flags for abs(X) * abs(X) and nabs(X) * nabs(X) Apr 14, 2024
@AZero13 AZero13 force-pushed the abs(x) branch 2 times, most recently from 204cabc to 4167191 Compare April 14, 2024 17:27
@goldsteinn
Copy link
Contributor

Alive2 Proofs:

https://alive2.llvm.org/ce/z/CN9BP- https://alive2.llvm.org/ce/z/kq9vh6 https://alive2.llvm.org/ce/z/LtVSL5

can you either start 1) labeling your links with which case they are proving or 2) putting all your proofs in the same file w/ useful postfixes for matching them to the code.

@goldsteinn
Copy link
Contributor

Think your first proof should be nsw, not nuw. You cover the nuw case in your third proof. Can you please be more diligent with your proofs.

@goldsteinn goldsteinn requested a review from dtcxzyw April 15, 2024 00:53
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.

Some of this code should be removed instead, done in #88675.

@nikic
Copy link
Contributor

nikic commented Apr 16, 2024

Please rebase over #88675 and remove the SPF and nabs handling.

@AZero13
Copy link
Contributor Author

AZero13 commented Apr 18, 2024

Please rebase over #88675 and remove the SPF and nabs handling.

Done!

@AZero13 AZero13 force-pushed the abs(x) branch 2 times, most recently from fcfaead to 6da3400 Compare April 18, 2024 14:27
@goldsteinn
Copy link
Contributor

LGTM although think the impl could be cleaned up a bit. Wait on another approval.

NewMul->setHasNoSignedWrap(true);

return NewMul;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unnecessary nabs handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nabs actually has a thing that doesn't apply to abs where if it is nuw, the transformation to x * x is both nuw and nsw.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am aware, but that does not mean that the extra code is useful.

It's okay to improve poison-propagation of an existing transform, but if you want to do more with that, you'll have to provide proof of real-world usefulness.

@AZero13 AZero13 closed this Jul 23, 2024
@AZero13 AZero13 deleted the abs(x) branch July 23, 2024 17:28
@AZero13 AZero13 restored the abs(x) branch July 23, 2024 17:29
@AZero13 AZero13 reopened this Jul 23, 2024
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.

@AZero13 AZero13 closed this Jul 29, 2024
@AZero13 AZero13 deleted the abs(x) branch July 29, 2024 19:37
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.

4 participants