[ValueTracking] Extend known bits of mul with self and constant
#81892
Conversation
InstCombine was previously suboptimal when computing `abs(b * a * a)` with `b` being a known constant. This has been addressed by computing the sign bit in `computeKnownBitsMul`, when multiplying a value with itself and a constant. Fixes: llvm#78018. Proofs: https://alive2.llvm.org/ce/z/E3uJum.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis Author: Antonio Frighetto (antoniofrighetto) ChangesInstCombine was previously suboptimal when computing Fixes: #78018. Proofs: https://alive2.llvm.org/ce/z/E3uJum. Full diff: https://github.com/llvm/llvm-project/pull/81892.diff 2 Files Affected:
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index cc1d5b74dcfc53..db26af467139c9 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -373,11 +373,24 @@ static void computeKnownBitsMul(const Value *Op0, const Value *Op1, bool NSW,
bool isKnownNegative = false;
bool isKnownNonNegative = false;
+ const APInt *C;
+ unsigned BitWidth = Known.getBitWidth();
+
// If the multiplication is known not to overflow, compute the sign bit.
if (NSW) {
if (Op0 == Op1) {
// The product of a number with itself is non-negative.
isKnownNonNegative = true;
+ } else if (((match(Op0, m_Mul(m_Specific(Op1), m_APInt(C))) &&
+ cast<OverflowingBinaryOperator>(Op0)->hasNoSignedWrap()) ||
+ (match(Op0, m_Shl(m_Specific(Op1), m_APInt(C))) &&
+ C->ult(BitWidth))) &&
+ !C->isZero() && !Known.isZero()) {
+ // The product of a number with itself and a constant depends on the sign
+ // of the constant.
+ KnownBits KnownC = KnownBits::makeConstant(*C);
+ isKnownNonNegative = KnownC.isNonNegative();
+ isKnownNegative = KnownC.isNegative();
} else {
bool isKnownNonNegativeOp1 = Known.isNonNegative();
bool isKnownNonNegativeOp0 = Known2.isNonNegative();
diff --git a/llvm/test/Transforms/InstCombine/mul.ll b/llvm/test/Transforms/InstCombine/mul.ll
index e7141d7c25ad21..656ed50fc25fd2 100644
--- a/llvm/test/Transforms/InstCombine/mul.ll
+++ b/llvm/test/Transforms/InstCombine/mul.ll
@@ -2049,3 +2049,64 @@ define i32 @zext_negpow2_use(i8 %x) {
%r = mul i32 %zx, -16777216 ; -1 << 24
ret i32 %r
}
+
+define i1 @self_with_constant_greater_than_zero(i8 %a) {
+; CHECK-LABEL: @self_with_constant_greater_than_zero(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: ret i1 true
+;
+entry:
+ %mul = mul nsw i8 %a, 3
+ %mul1 = mul nsw i8 %mul, %a
+ %cmp = icmp sge i8 %mul1, 0
+ ret i1 %cmp
+}
+
+define i8 @abs_of_self_with_constant(i8 %a) {
+; CHECK-LABEL: @abs_of_self_with_constant(
+; CHECK-NEXT: [[MUL:%.*]] = shl nsw i8 [[A:%.*]], 1
+; CHECK-NEXT: [[MUL1:%.*]] = mul nsw i8 [[MUL]], [[A]]
+; CHECK-NEXT: ret i8 [[MUL1]]
+;
+ %mul = mul nsw i8 %a, 2
+ %mul1 = mul nsw i8 %mul, %a
+ %r = tail call i8 @llvm.abs.i8(i8 %mul1, i1 true)
+ ret i8 %r
+}
+
+define i8 @abs_of_self_with_constant_neg_constant(i8 %a) {
+; CHECK-LABEL: @abs_of_self_with_constant_neg_constant(
+; CHECK-NEXT: [[MUL_NEG:%.*]] = mul i8 [[A:%.*]], 3
+; CHECK-NEXT: [[MUL1_NEG:%.*]] = mul nsw i8 [[MUL_NEG]], [[A]]
+; CHECK-NEXT: ret i8 [[MUL1_NEG]]
+;
+ %mul = mul nsw i8 %a, -3
+ %mul1 = mul nsw i8 %mul, %a
+ %r = tail call i8 @llvm.abs.i8(i8 %mul1, i1 true)
+ ret i8 %r
+}
+
+define i8 @abs_of_self_with_constant_2_inv_ops(i8 %a) {
+; CHECK-LABEL: @abs_of_self_with_constant_2_inv_ops(
+; CHECK-NEXT: [[MUL:%.*]] = shl nsw i8 [[A:%.*]], 2
+; CHECK-NEXT: [[MUL1:%.*]] = mul nsw i8 [[MUL]], [[A]]
+; CHECK-NEXT: ret i8 [[MUL1]]
+;
+ %mul = mul nsw i8 4, %a
+ %mul1 = mul nsw i8 %a, %mul
+ %r = tail call i8 @llvm.abs.i8(i8 %mul1, i1 true)
+ ret i8 %r
+}
+
+define i8 @abs_of_self_with_constant_no_nsw(i8 %a) {
+; CHECK-LABEL: @abs_of_self_with_constant_no_nsw(
+; CHECK-NEXT: [[MUL:%.*]] = mul i8 [[A:%.*]], 3
+; CHECK-NEXT: [[MUL1:%.*]] = mul nsw i8 [[MUL]], [[A]]
+; CHECK-NEXT: [[R:%.*]] = tail call i8 @llvm.abs.i8(i8 [[MUL1]], i1 true)
+; CHECK-NEXT: ret i8 [[R]]
+;
+ %mul = mul i8 %a, 3
+ %mul1 = mul nsw i8 %mul, %a
+ %r = tail call i8 @llvm.abs.i8(i8 %mul1, i1 true)
+ ret i8 %r
+}
|
| // If the multiplication is known not to overflow, compute the sign bit. | ||
| if (NSW) { | ||
| if (Op0 == Op1) { | ||
| // The product of a number with itself is non-negative. | ||
| isKnownNonNegative = true; | ||
| } else if (((match(Op0, m_Mul(m_Specific(Op1), m_APInt(C))) && | ||
| cast<OverflowingBinaryOperator>(Op0)->hasNoSignedWrap()) || | ||
| (match(Op0, m_Shl(m_Specific(Op1), m_APInt(C))) && |
There was a problem hiding this comment.
Why limit this to constant. Why not just match m_Value(X)?
| // of the constant. | ||
| KnownBits KnownC = KnownBits::makeConstant(*C); | ||
| isKnownNonNegative = KnownC.isNonNegative(); | ||
| isKnownNegative = KnownC.isNegative(); |
There was a problem hiding this comment.
Not sure if this is correct: https://alive2.llvm.org/ce/z/K98HUL
can you please add generalized proofs instead of just the select constant cases you have?
There was a problem hiding this comment.
Uhmm, right. So the first one is fine, the second one not really: https://alive2.llvm.org/ce/z/9kAM_g (simplified by simplifyICmpWithZero). We could consider only isKnownNonNegative, but we wouldn't catch the first case.
There was a problem hiding this comment.
@goldsteinn Any suggestion on how this should be better handled?
There was a problem hiding this comment.
I would start with a proof similiar to https://alive2.llvm.org/ce/z/K98HUL where instead of testing a specific constants and specifically checking the sge/slt cases, it implements the generalized logic you are trying to implement in C++.
From the looks of things (although there may be a bug in my "proof"), the generalized concept is not correct, in which case you can constraint the %C argument using llvm.assume(i1). Although, additional constants on %C only further nikic's argument that this is hacky, so i think there is a reasonable chance this change will need to be abandoned unless you can find/show some realworld examples where it makes an impact.
There was a problem hiding this comment.
The generalized concept indeed doesn't look correct. I thought about constraining %C to fit in the type signed integer range, but looks a bit trickery and indeed further @nikic's concern. I'm unsure too, OTOH the patch would be quite small and there seems to be other specific cases handled similarly there in VT.
There was a problem hiding this comment.
There are no real hard rules for what gets in. It mostly up to the judgement of whoever is reviewing/the community. Generally there is a tradeoff between code maintainability+compilation speed vs compilation quality. If its considered unlikely that the compilation quality aspect will ever actually pay off (no realworld case), the maintainability+compilation speed will often win out. But then again, reviewers/the community can be capricious, so there will always be exceptions. Either way the primary reviewer of this (nikic) has made his opinion clear, so if you want to get this in I would:
- Make it correct
- Try compiling a few codebases with it and see if it ever actually applies. If it does, link to the code snippets where it applies and it will make you case much stronger.
|
TBH, I am very skeptical about fixing these reassociation issues by introducing hacks for specific cases. I've seen a few variants of this (though, I believe, always only in constructed examples, never motivated by real-world need) and if we want to fix them, we should probably fix them by reassociating in the other direction in InstCombine. |
InstCombine was previously suboptimal when computing
abs(b * a * a)withbbeing a known constant.This has been addressed by computing the sign bit in
computeKnownBitsMul, when multiplying a value with itself and a constant.Fixes: #78018.
Proofs: https://alive2.llvm.org/ce/z/E3uJum.