-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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] Remove mul of SPF abs fold #88675
Conversation
@llvm/pr-subscribers-llvm-transforms Author: Nikita Popov (nikic) ChangesRemove the fold working on abs in SPF representation now that we canonicalize SPF to intrinsics. This is not strictly NFC because the SPF fold might fire for non-canonical IR due to multi-use, but given the lack of test coverage, I assume this is not important. Full diff: https://github.com/llvm/llvm-project/pull/88675.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
index 4dc1319f1c437f..ea3443175568c5 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
@@ -319,19 +319,13 @@ Instruction *InstCombinerImpl::visitMul(BinaryOperator &I) {
}
// abs(X) * abs(X) -> X * X
- // nabs(X) * nabs(X) -> X * X
- 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 (match(Op0, m_Intrinsic<Intrinsic::abs>(m_Value(X))))
- return BinaryOperator::CreateMul(X, X);
- }
+ Value *X;
+ if (Op0 == Op1 &&
+ match(Op0, m_Intrinsic<Intrinsic::abs>(m_Value(X))))
+ return BinaryOperator::CreateMul(X, X);
{
- Value *X, *Y;
+ Value *Y;
// abs(X) * abs(Y) -> abs(X * Y)
if (I.hasNoSignedWrap() &&
match(Op0,
@@ -344,7 +338,7 @@ Instruction *InstCombinerImpl::visitMul(BinaryOperator &I) {
}
// -X * C --> X * -C
- Value *X, *Y;
+ Value *Y;
Constant *Op1C;
if (match(Op0, m_Neg(m_Value(X))) && match(Op1, m_Constant(Op1C)))
return BinaryOperator::CreateMul(X, ConstantExpr::getNeg(Op1C));
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Remove the fold working on abs in SPF representation now that we canonicalize SPF to intrinsics. This is not strictly NFC because the SPF fold might fire for non-canonical IR due to multi-use, but given the lack of test coverage, I assume this is not important.
89e26d4
to
562e199
Compare
Assuming no regression in @dtcxzyw tests, LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
} | ||
Value *X; | ||
if (Op0 == Op1 && match(Op0, m_Intrinsic<Intrinsic::abs>(m_Value(X)))) | ||
return BinaryOperator::CreateMul(X, X); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can propagate nsw flag as the following code does.
Alive2: https://alive2.llvm.org/ce/z/uQpZJJ
FYI this pattern is rare: https://dtcxzyw.github.io/llvm-opt-benchmark/coverage/home/dtcxzyw/llvm-project/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp.html#L321
For nabs we can propagate nsw if nuw is present see the alive proofs here |
Remove the fold working on abs in SPF representation now that we canonicalize SPF to intrinsics.
This is not strictly NFC because the SPF fold might fire for non-canonical IR due to multi-use, but given the lack of test coverage, I assume this is not important.