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] Simplifiy sdiv -X, X into X == INT_MIN ? 1 : -1 #71768

Merged
merged 4 commits into from
Nov 15, 2023

Conversation

Z572
Copy link
Contributor

@Z572 Z572 commented Nov 9, 2023

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 9, 2023

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Z572 (Z572)

Changes

Fixes #69574


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp (+14)
  • (modified) llvm/test/Transforms/InstCombine/div.ll (+30)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
index db0804380855e3a..a4460f0b3020918 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
@@ -1544,6 +1544,20 @@ Instruction *InstCombinerImpl::visitSDiv(BinaryOperator &I) {
     }
   }
 
+  // -(X * Y) / (X * Y) --> -1
+  if ((match(Op0, m_Neg(m_c_Mul(m_Value(X), m_Value(Y)))) &&
+       match(Op1, m_c_Mul(m_Specific(X), m_Specific(Y)))) ||
+      (match(Op0, m_c_Mul(m_Value(X), m_Value(Y))) &&
+       match(Op1, m_Neg(m_c_Mul(m_Specific(X), m_Specific(Y)))))) {
+    return BinaryOperator::CreateNeg(ConstantInt::get(Ty, 1));
+  };
+
+  // (Y - X) / (X - Y) --> -1
+  if (match(Op0, m_Sub(m_Value(Y), m_Value(X))) &&
+      match(Op1, m_Sub(m_Specific(X), m_Specific(Y)))) {
+    return BinaryOperator::CreateNeg(ConstantInt::get(Ty, 1));
+  }
+
   return nullptr;
 }
 
diff --git a/llvm/test/Transforms/InstCombine/div.ll b/llvm/test/Transforms/InstCombine/div.ll
index 1f0081befb07dba..099c12d60f248df 100644
--- a/llvm/test/Transforms/InstCombine/div.ll
+++ b/llvm/test/Transforms/InstCombine/div.ll
@@ -1432,6 +1432,36 @@ define <2 x i8> @sdiv_sdiv_mul_nsw(<2 x i8> %x, <2 x i8> %y, <2 x i8> %z) {
   ret <2 x i8> %r
 }
 
+; -(X * Y) / (X * Y) --> -1
+define i32 @sdiv_neg_mul_mul(i32 %0, i32 %1) {
+; CHECK-LABEL: @sdiv_neg_mul_mul(
+; CHECK-NEXT:    ret i32 -1
+  %3 = mul i32 %1, %0
+  %4 = sub i32 0, %3
+  %5 = sdiv i32 %4, %3
+  ret i32 %5
+}
+
+; (X * Y) / -(X * Y) --> -1
+define i32 @sdiv_mul_neg_mul(i32 %0, i32 %1) {
+; CHECK-LABEL: @sdiv_mul_neg_mul(
+; CHECK-NEXT:    ret i32 -1
+  %3 = mul i32 %1, %0
+  %4 = sub i32 0, %3
+  %5 = sdiv i32 %3, %4
+  ret i32 %5
+}
+
+; (Y - X) / (X - Y) --> -1
+define i32 @sdiv_sub_sub(i32 %0, i32 %1) {
+; CHECK-LABEL: @sdiv_sub_sub(
+; CHECK-NEXT:    ret i32 -1
+  %3 = sub i32 %1, %0
+  %4 = sub nsw i32 %0, %1
+  %5 = sdiv i32 %3, %4
+  ret i32 %5
+}
+
 ; exact propagates
 
 define i8 @sdiv_sdiv_mul_nsw_exact_exact(i8 %x, i8 %y, i8 %z) {

@dtcxzyw
Copy link
Member

dtcxzyw commented Nov 9, 2023

Please attach alive2 proof.
This transform is incorrect when both the divisor and dividend are equal to INT_MIN.
Alive2: https://alive2.llvm.org/ce/z/VBRrGU

llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp Outdated Show resolved Hide resolved
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp Outdated Show resolved Hide resolved
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp Outdated Show resolved Hide resolved
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp Outdated Show resolved Hide resolved
llvm/test/Transforms/InstCombine/div.ll Outdated Show resolved Hide resolved
@dtcxzyw
Copy link
Member

dtcxzyw commented Nov 9, 2023

Alive2 proof: https://alive2.llvm.org/ce/z/AYRxRU

Copy link

github-actions bot commented Nov 13, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@Z572 Z572 changed the title [InstCombine] Simplification for (-a * b) / (a * b) and (a - b) / (b - a). [InstCombine] Simplification for (-a * b) / (a * b). Nov 13, 2023
@Z572 Z572 force-pushed the patch-3 branch 2 times, most recently from 192489c to 0dffd8d Compare November 13, 2023 14:47
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.

As far as I can tell, all the tests you have added already fold without the patch: https://llvm.godbolt.org/z/z4W9KhP3b

Please adjust the test coverage to check cases that are actually newly supported with this patch.

llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp Outdated Show resolved Hide resolved
llvm/test/Transforms/InstCombine/div.ll Outdated Show resolved Hide resolved
llvm/test/Transforms/InstCombine/div.ll Outdated Show resolved Hide resolved
@nikic
Copy link
Contributor

nikic commented Nov 14, 2023

I think the transform you actually want to implement is this one: https://alive2.llvm.org/ce/z/PBjgSM

The cases from #69574 lack the nsw flag, and this is the variant of the transform that doesn't need nsw.

@dtcxzyw dtcxzyw changed the title [InstCombine] Simplification for (-a * b) / (a * b). [InstCombine] Simplifiy sdiv -X, X into X == INT_MIN ? 1 : -1 Nov 15, 2023
%neg = sub i32 0, %arg
%div = sdiv i32 %arg, %neg
ret i32 %div
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add a multi-use test (extra use of %neg). As a div is removed, we want to allow the transform with multi-use.

Please also test the case (x - y) / (y - x), which is one of the special cases isKnownNegation() handles.

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 apart from the final test nit.

llvm/test/Transforms/InstCombine/div.ll Outdated Show resolved Hide resolved
llvm/test/Transforms/InstCombine/div.ll Outdated Show resolved Hide resolved
@Z572 Z572 merged commit c350a1e into llvm:main Nov 15, 2023
3 checks passed
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
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.

Simplification for (-a * b) / (a * b) and (-a + b) / (a - b)
5 participants