-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[InstCombine] Fix missed opportunity to fold 'or' into 'mul' operand. #74225
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
Conversation
… operand. NFC We are able to fold or (mul X, Y), X --> mul X, (add Y, 1) (when the multiply has no common bits with X) but we miss it if the mul operands are commuted.
We were able to fold or (mul X, Y), X --> mul X, (add Y, 1) (when the multiply has no common bits with X) This patch makes the transform work if the mul operands are commuted.
@llvm/pr-subscribers-llvm-transforms Author: Craig Topper (topperc) ChangesWe were able to fold This patch makes the transform work if the mul operands are commuted. Full diff: https://github.com/llvm/llvm-project/pull/74225.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index 7379bdf93169e..50d20eb2f97b2 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -3394,7 +3394,7 @@ Instruction *InstCombinerImpl::visitOr(BinaryOperator &I) {
// If the operands have no common bits set:
// or (mul X, Y), X --> add (mul X, Y), X --> mul X, (Y + 1)
if (match(&I,
- m_c_Or(m_OneUse(m_Mul(m_Value(X), m_Value(Y))), m_Deferred(X))) &&
+ m_c_Or(m_Value(X), m_OneUse(m_c_Mul(m_Deferred(X), m_Value(Y))))) &&
haveNoCommonBitsSet(Op0, Op1, DL)) {
Value *IncrementY = Builder.CreateAdd(Y, ConstantInt::get(Ty, 1));
return BinaryOperator::CreateMul(X, IncrementY);
diff --git a/llvm/test/Transforms/InstCombine/or.ll b/llvm/test/Transforms/InstCombine/or.ll
index 8bf8c6fcd928b..81ef38d43e8c3 100644
--- a/llvm/test/Transforms/InstCombine/or.ll
+++ b/llvm/test/Transforms/InstCombine/or.ll
@@ -1511,6 +1511,21 @@ define <2 x i12> @mul_no_common_bits_commute(<2 x i12> %p) {
ret <2 x i12> %r
}
+define i32 @mul_no_common_bits_commute2(i32 %p1, i32 %p2) {
+; CHECK-LABEL: @mul_no_common_bits_commute2(
+; CHECK-NEXT: [[X:%.*]] = and i32 [[P1:%.*]], 7
+; CHECK-NEXT: [[Y:%.*]] = shl i32 [[P2:%.*]], 3
+; CHECK-NEXT: [[TMP1:%.*]] = or disjoint i32 [[Y]], 1
+; CHECK-NEXT: [[R:%.*]] = mul i32 [[X]], [[TMP1]]
+; CHECK-NEXT: ret i32 [[R]]
+;
+ %x = and i32 %p1, 7
+ %y = shl i32 %p2, 3
+ %m = mul i32 %y, %x
+ %r = or i32 %m, %x
+ ret i32 %r
+}
+
; negative test - extra use requires extra instructions
define i32 @mul_no_common_bits_uses(i32 %p1, i32 %p2) {
|
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
It looks like this caused an infinite loop in the stage2 clang build. |
I reverted it. Any idea why its looping? |
What if X is a constant? |
That appears to be the issue. I might look into seeing if I can make foldUsingDistributiveLaws handle disjoint or+mul like add+mul directly. Instead of approximating here. |
We were able to fold
or (mul X, Y), X --> mul X, (add Y, 1) (when the multiply has no common bits with X)
This patch makes the transform work if the mul operands are commuted.