[ValueTracking] Improve KnownBits for signed min-max clamping #120576
Conversation
A signed min-max clamp is the sequence of smin and smax intrinsics, which constrain a signed value into the range: smin <= value <= smax. The patch improves the calculation of KnownBits for a value subjected to the signed clamping.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-llvm-analysis Author: None (adam-bzowski) ChangesA signed min-max clamp is the sequence of smin and smax intrinsics, which constrain a signed value into the range: smin <= value <= smax. The patch improves the calculation of KnownBits for a value subjected to the signed clamping. Full diff: https://github.com/llvm/llvm-project/pull/120576.diff 2 Files Affected:
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 14d7c2da8a9f8e..dad1c2bb2347ef 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1065,6 +1065,62 @@ void llvm::adjustKnownBitsForSelectArm(KnownBits &Known, Value *Cond,
Known = CondRes;
}
+// Match a signed min+max clamp pattern like smax(smin(In, CHigh), CLow).
+// Returns the input and lower/upper bounds.
+static bool isSignedMinMaxClamp(const Value *Select, const Value *&In,
+ const APInt *&CLow, const APInt *&CHigh) {
+ assert(isa<Operator>(Select) &&
+ cast<Operator>(Select)->getOpcode() == Instruction::Select &&
+ "Input should be a Select!");
+
+ const Value *LHS = nullptr, *RHS = nullptr;
+ SelectPatternFlavor SPF = matchSelectPattern(Select, LHS, RHS).Flavor;
+ if (SPF != SPF_SMAX && SPF != SPF_SMIN)
+ return false;
+
+ if (!match(RHS, m_APInt(CLow)))
+ return false;
+
+ const Value *LHS2 = nullptr, *RHS2 = nullptr;
+ SelectPatternFlavor SPF2 = matchSelectPattern(LHS, LHS2, RHS2).Flavor;
+ if (getInverseMinMaxFlavor(SPF) != SPF2)
+ return false;
+
+ if (!match(RHS2, m_APInt(CHigh)))
+ return false;
+
+ if (SPF == SPF_SMIN)
+ std::swap(CLow, CHigh);
+
+ In = LHS2;
+ return CLow->sle(*CHigh);
+}
+
+static bool isSignedMinMaxIntrinsicClamp(const IntrinsicInst *II,
+ const APInt *&CLow,
+ const APInt *&CHigh) {
+ assert((II->getIntrinsicID() == Intrinsic::smin ||
+ II->getIntrinsicID() == Intrinsic::smax) && "Must be smin/smax");
+
+ Intrinsic::ID InverseID = getInverseMinMaxIntrinsic(II->getIntrinsicID());
+ auto *InnerII = dyn_cast<IntrinsicInst>(II->getArgOperand(0));
+ if (!InnerII || InnerII->getIntrinsicID() != InverseID ||
+ !match(II->getArgOperand(1), m_APInt(CLow)) ||
+ !match(InnerII->getArgOperand(1), m_APInt(CHigh)))
+ return false;
+
+ if (II->getIntrinsicID() == Intrinsic::smin)
+ std::swap(CLow, CHigh);
+ return CLow->sle(*CHigh);
+}
+
+static void unionWithMinMaxIntrinsicClamp(const IntrinsicInst *II,
+ KnownBits &Known) {
+ const APInt *CLow, *CHigh;
+ if (isSignedMinMaxIntrinsicClamp(II, CLow, CHigh))
+ Known = Known.unionWith(ConstantRange(*CLow, *CHigh).toKnownBits());
+}
+
static void computeKnownBitsFromOperator(const Operator *I,
const APInt &DemandedElts,
KnownBits &Known, unsigned Depth,
@@ -1804,11 +1860,13 @@ static void computeKnownBitsFromOperator(const Operator *I,
computeKnownBits(I->getOperand(0), DemandedElts, Known, Depth + 1, Q);
computeKnownBits(I->getOperand(1), DemandedElts, Known2, Depth + 1, Q);
Known = KnownBits::smin(Known, Known2);
+ unionWithMinMaxIntrinsicClamp(II, Known);
break;
case Intrinsic::smax:
computeKnownBits(I->getOperand(0), DemandedElts, Known, Depth + 1, Q);
computeKnownBits(I->getOperand(1), DemandedElts, Known2, Depth + 1, Q);
Known = KnownBits::smax(Known, Known2);
+ unionWithMinMaxIntrinsicClamp(II, Known);
break;
case Intrinsic::ptrmask: {
computeKnownBits(I->getOperand(0), DemandedElts, Known, Depth + 1, Q);
@@ -3751,55 +3809,6 @@ static bool isKnownNonEqual(const Value *V1, const Value *V2,
return false;
}
-// Match a signed min+max clamp pattern like smax(smin(In, CHigh), CLow).
-// Returns the input and lower/upper bounds.
-static bool isSignedMinMaxClamp(const Value *Select, const Value *&In,
- const APInt *&CLow, const APInt *&CHigh) {
- assert(isa<Operator>(Select) &&
- cast<Operator>(Select)->getOpcode() == Instruction::Select &&
- "Input should be a Select!");
-
- const Value *LHS = nullptr, *RHS = nullptr;
- SelectPatternFlavor SPF = matchSelectPattern(Select, LHS, RHS).Flavor;
- if (SPF != SPF_SMAX && SPF != SPF_SMIN)
- return false;
-
- if (!match(RHS, m_APInt(CLow)))
- return false;
-
- const Value *LHS2 = nullptr, *RHS2 = nullptr;
- SelectPatternFlavor SPF2 = matchSelectPattern(LHS, LHS2, RHS2).Flavor;
- if (getInverseMinMaxFlavor(SPF) != SPF2)
- return false;
-
- if (!match(RHS2, m_APInt(CHigh)))
- return false;
-
- if (SPF == SPF_SMIN)
- std::swap(CLow, CHigh);
-
- In = LHS2;
- return CLow->sle(*CHigh);
-}
-
-static bool isSignedMinMaxIntrinsicClamp(const IntrinsicInst *II,
- const APInt *&CLow,
- const APInt *&CHigh) {
- assert((II->getIntrinsicID() == Intrinsic::smin ||
- II->getIntrinsicID() == Intrinsic::smax) && "Must be smin/smax");
-
- Intrinsic::ID InverseID = getInverseMinMaxIntrinsic(II->getIntrinsicID());
- auto *InnerII = dyn_cast<IntrinsicInst>(II->getArgOperand(0));
- if (!InnerII || InnerII->getIntrinsicID() != InverseID ||
- !match(II->getArgOperand(1), m_APInt(CLow)) ||
- !match(InnerII->getArgOperand(1), m_APInt(CHigh)))
- return false;
-
- if (II->getIntrinsicID() == Intrinsic::smin)
- std::swap(CLow, CHigh);
- return CLow->sle(*CHigh);
-}
-
/// For vector constants, loop over the elements and find the constant with the
/// minimum number of sign bits. Return 0 if the value is not a vector constant
/// or if any element was not analyzed; otherwise, return the count for the
diff --git a/llvm/test/Analysis/ValueTracking/knownbits-trunc-with-min-max-clamp.ll b/llvm/test/Analysis/ValueTracking/knownbits-trunc-with-min-max-clamp.ll
new file mode 100644
index 00000000000000..aa01a0d3f3ab8e
--- /dev/null
+++ b/llvm/test/Analysis/ValueTracking/knownbits-trunc-with-min-max-clamp.ll
@@ -0,0 +1,100 @@
+; RUN: opt < %s -passes=aggressive-instcombine -mtriple=x86_64 -S | FileCheck %s
+
+; This LIT test checks if TruncInstCombine pass correctly recognizes the
+; constraints from a signed min-max clamp. The clamp is a sequence of smin and
+; smax instructions limiting a variable into a range, smin <= x <= smax.
+
+declare i16 @llvm.smin.i16(i16, i16)
+declare i16 @llvm.smax.i16(i16, i16)
+
+
+; CHECK-LABEL: @test_1
+; CHECK-NEXT: [[ONE:%.*]] = tail call i16 @llvm.smin.i16(i16 [[X:%.*]], i16 31)
+; CHECK-NEXT: [[TWO:%.*]] = tail call i16 @llvm.smax.i16(i16 [[ONE]], i16 0)
+; CHECK-NEXT: [[A:%.*]] = trunc i16 [[TWO]] to i8
+; CHECK-NEXT: [[B:%.*]] = lshr i8 [[A]], 2
+; CHECK-NEXT: ret i8 [[B]]
+
+define i8 @test_1(i16 %x) {
+ %1 = tail call i16 @llvm.smin.i16(i16 %x, i16 31)
+ %2 = tail call i16 @llvm.smax.i16(i16 %1, i16 0)
+ %a = sext i16 %2 to i32
+ %b = lshr i32 %a, 2
+ %b.trunc = trunc i32 %b to i8
+ ret i8 %b.trunc
+}
+
+
+; CHECK-LABEL: @test_1a
+; CHECK-NEXT: [[ONE:%.*]] = tail call i16 @llvm.smin.i16(i16 [[X:%.*]], i16 31)
+; CHECK-NEXT: [[TWO:%.*]] = tail call i16 @llvm.smax.i16(i16 [[ONE]], i16 0)
+; CHECK-NEXT: [[A:%.*]] = trunc i16 [[TWO]] to i8
+; CHECK-NEXT: [[B:%.*]] = add i8 [[A]], 2
+; CHECK-NEXT: ret i8 [[B]]
+
+define i8 @test_1a(i16 %x) {
+ %1 = tail call i16 @llvm.smin.i16(i16 %x, i16 31)
+ %2 = tail call i16 @llvm.smax.i16(i16 %1, i16 0)
+ %a = sext i16 %2 to i32
+ %b = add i32 %a, 2
+ %b.trunc = trunc i32 %b to i8
+ ret i8 %b.trunc
+}
+
+
+; CHECK-LABEL: @test_2
+; CHECK-NEXT: [[ONE:%.*]] = tail call i16 @llvm.smin.i16(i16 [[X:%.*]], i16 -1)
+; CHECK-NEXT: [[TWO:%.*]] = tail call i16 @llvm.smax.i16(i16 [[ONE]], i16 -31)
+; CHECK-NEXT: [[A:%.*]] = trunc i16 [[TWO]] to i8
+; CHECK-NEXT: [[B:%.*]] = add i8 [[A]], 2
+; CHECK-NEXT: ret i8 [[B]]
+
+define i8 @test_2(i16 %x) {
+ %1 = tail call i16 @llvm.smin.i16(i16 %x, i16 -1)
+ %2 = tail call i16 @llvm.smax.i16(i16 %1, i16 -31)
+ %a = sext i16 %2 to i32
+ %b = add i32 %a, 2
+ %b.trunc = trunc i32 %b to i8
+ ret i8 %b.trunc
+}
+
+
+; CHECK-LABEL: @test_3
+; CHECK-NEXT: [[ONE:%.*]] = tail call i16 @llvm.smin.i16(i16 [[X:%.*]], i16 31)
+; CHECK-NEXT: [[TWO:%.*]] = tail call i16 @llvm.smax.i16(i16 [[ONE]], i16 -31)
+; CHECK-NEXT: [[A:%.*]] = trunc i16 [[TWO]] to i8
+; CHECK-NEXT: [[B:%.*]] = add i8 [[A]], 2
+; CHECK-NEXT: ret i8 [[B]]
+
+define i8 @test_3(i16 %x) {
+ %1 = tail call i16 @llvm.smin.i16(i16 %x, i16 31)
+ %2 = tail call i16 @llvm.smax.i16(i16 %1, i16 -31)
+ %a = sext i16 %2 to i32
+ %b = add i32 %a, 2
+ %b.trunc = trunc i32 %b to i8
+ ret i8 %b.trunc
+}
+
+
+; CHECK-LABEL: @test_4
+; CHECK-NEXT: [[ONE:%.*]] = tail call i16 @llvm.smin.i16(i16 [[X:%.*]], i16 127)
+; CHECK-NEXT: [[TWO:%.*]] = tail call i16 @llvm.smax.i16(i16 [[ONE]], i16 0)
+; CHECK-NEXT: [[THREE:%.*]] = tail call i16 @llvm.smin.i16(i16 [[Y:%.*]], i16 127)
+; CHECK-NEXT: [[FOUR:%.*]] = tail call i16 @llvm.smax.i16(i16 [[THREE]], i16 0)
+; CHECK-NEXT: [[A:%.*]] = mul i16 [[TWO]], [[FOUR]]
+; CHECK-NEXT: [[B:%.*]] = lshr i16 [[A]], 7
+; CHECK-NEXT: [[C:%.*]] = trunc i16 [[B]] to i8
+; CHECK-NEXT: ret i8 [[C]]
+
+define i8 @test_4(i16 %x, i16 %y) {
+ %1 = tail call i16 @llvm.smin.i16(i16 %x, i16 127)
+ %2 = tail call i16 @llvm.smax.i16(i16 %1, i16 0)
+ %x.clamp = zext nneg i16 %2 to i32
+ %3 = tail call i16 @llvm.smin.i16(i16 %y, i16 127)
+ %4 = tail call i16 @llvm.smax.i16(i16 %3, i16 0)
+ %y.clamp = zext nneg i16 %4 to i32
+ %mul = mul nuw nsw i32 %x.clamp, %y.clamp
+ %shr = lshr i32 %mul, 7
+ %trunc= trunc nuw nsw i32 %shr to i8
+ ret i8 %trunc
+}
|
|
This is a reworking of the solution to the problem investigated in this PR: #119577. Its original goal was reformulated to the problem of type narrowing in the presence of a signed min-max clamp. A signed min-max clamp is the sequence of smin and smax intrinsics, which constrain a signed value into the range: smin <= value <= smax. The patch improves the calculation of KnownBits for a value subjected to the signed clamping. When calculating KnownBits, whenever smin or smax is encountered, the patch checks if it is a part of the smin-smax clamp. If it is the case, it narrows the KnownBits accordingly. To see why such an improvement was needed, consider a simple clamp of the form y = smax(smin(x, 0xFF), 0) for a signed variable x. Without the patch, KnownBits cannot infer anything about smin(x, 0xFF) and thus it only treats y as non-negative (leading bit = 0). With the patch, KnownBits correctly identifies all the leading bits but the 8 lowest as vanishing. The submission essentially consists of 6 additional lines of code (definition of unionWithMinMaxIntrinsicClamp and 2 calls). The remaining change is moving the definition of isSignedMinMaxIntrinsicClamp before its use. |
A signed min-max clamp is the sequence of smin and smax intrinsics, which constrain a signed value into the range: smin <= value <= smax. The patch improves the calculation of KnownBits for a value subjected to the signed clamping.
llvm/test/Analysis/ValueTracking/knownbits-trunc-with-min-max-clamp.ll
Outdated
Show resolved
Hide resolved
llvm/test/Analysis/ValueTracking/knownbits-trunc-with-min-max-clamp.ll
Outdated
Show resolved
Hide resolved
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
A signed min-max clamp is the sequence of smin and smax intrinsics, which constrain a signed value into the range: smin <= value <= smax. The patch improves the calculation of KnownBits for a value subjected to the signed clamping.
This is fixed as well. Please let me know if I should do something more. Thanks. |
llvm/lib/Analysis/ValueTracking.cpp
Outdated
| KnownBits &Known) { | ||
| const APInt *CLow, *CHigh; | ||
| if (isSignedMinMaxIntrinsicClamp(II, CLow, CHigh)) | ||
| Known = Known.unionWith(ConstantRange(*CLow, *CHigh).toKnownBits()); |
There was a problem hiding this comment.
| Known = Known.unionWith(ConstantRange(*CLow, *CHigh).toKnownBits()); | |
| Known = Known.unionWith(ConstantRange(*CLow, *CHigh + 1).toKnownBits()); |
*CHigh should be included by ConstantRange.
There was a problem hiding this comment.
It produces a wrong KnownBits result 00000000000????? for smax(smin(X, 32), 0).
There was a problem hiding this comment.
Here is an counterexample: https://alive2.llvm.org/ce/z/ji-_Jq
; bin/opt -passes=instcombine test.ll -S
define i16 @test_0a(i16 %x) {
%1 = tail call i16 @llvm.smin.i16(i16 %x, i16 32)
%2 = tail call i16 @llvm.smax.i16(i16 %1, i16 0)
%and = and i16 %2, 31
ret i16 %and
}
Output:
define i16 @test_0a(i16 %x) {
%1 = tail call i16 @llvm.smin.i16(i16 %x, i16 32)
%2 = tail call i16 @llvm.smax.i16(i16 %1, i16 0)
ret i16 %2
}
%and should not be eliminated.
Please add this negative test.
There was a problem hiding this comment.
Indeed, my mistake. I also checked that this is ok if *CHigh is MaxSignedValue (in this case +1 loops to MinSignedValue and ConstantRange interprets this correctly). I added 3 more LIT tests: two to check that +1 is correct now and one to verify that MaxSignedValue is dealt with correctly.
A signed min-max clamp is the sequence of smin and smax intrinsics, which constrain a signed value into the range: smin <= value <= smax. The patch improves the calculation of KnownBits for a value subjected to the signed clamping.
| @@ -0,0 +1,263 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5 | |||
| ; RUN: opt < %s -passes=aggressive-instcombine -mtriple=x86_64 -S | FileCheck %s | |||
There was a problem hiding this comment.
| ; RUN: opt < %s -passes=aggressive-instcombine -mtriple=x86_64 -S | FileCheck %s | |
| ; RUN: opt < %s -passes=aggressive-instcombine -S | FileCheck %s |
Please add an appropriate data layout instead, something like target datalayout = "n:32:16:8" probably works.
There was a problem hiding this comment.
Added target datalayout = "n8:16:32", removed -mtriple.
A signed min-max clamp is the sequence of smin and smax intrinsics, which constrain a signed value into the range: smin <= value <= smax. The patch improves the calculation of KnownBits for a value subjected to the signed clamping.
|
@adam-bzowski Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/81/builds/3367 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/17/builds/4892 Here is the relevant piece of the build log for the reference |
| KnownBits &Known) { | ||
| const APInt *CLow, *CHigh; | ||
| if (isSignedMinMaxIntrinsicClamp(II, CLow, CHigh)) | ||
| Known = Known.unionWith(ConstantRange(*CLow, *CHigh + 1).toKnownBits()); |
There was a problem hiding this comment.
The ConstantRange constructor will assert if CLow and CHigh+1 are the same value.
For example, this unittest I threw together
diff --git a/llvm/unittests/Analysis/ValueTrackingTest.cpp b/llvm/unittests/Analysis/ValueTrackingTest.cpp
index 0145ee70a14c..889f6bd5e78a 100644
--- a/llvm/unittests/Analysis/ValueTrackingTest.cpp
+++ b/llvm/unittests/Analysis/ValueTrackingTest.cpp
@@ -1262,6 +1262,16 @@ TEST_F(ComputeKnownBitsTest, ComputeKnownBits) {
expectKnownBits(/*zero*/ 4278190085u, /*one*/ 10u);
}
+TEST_F(ComputeKnownBitsTest, ComputeKnownBitsClamp) {
+ parseAssembly(
+ "define i32 @test(i32 %a) {\n"
+ " %amin = tail call i32 @llvm.smin.i32(i32 %a, i32 2147483647)\n"
+ " %A = tail call i32 @llvm.smax.i32(i32 %amin, i32 2147483648)\n"
+ " ret i32 %A\n"
+ "}\n");
+ expectKnownBits(/*zero*/ 0u, /*one*/ 0u);
+}
+
TEST_F(ComputeKnownBitsTest, ComputeKnownMulBits) {
parseAssembly(
"define i32 @test(i32 %a, i32 %b) {\n"
There was a problem hiding this comment.
Nice catch, this should be using ConstantRange::getNonEmpty().
There was a problem hiding this comment.
This is indeed a bug, I should have added "if" from the very beginning. I made the correction and added further LIT tests to check if everything works fine. Also, it turned out that TruncInstCombine.cpp uses the constraint from the min-max clamp only for some and not all binary operators. I used lshr in the lit tests and this definitely fires the min-max clamp constraint.
There was a problem hiding this comment.
Here's the fix: #121206. I'm still playing with some tests. Thanks for the comment and sorry for the trouble.
There was a problem hiding this comment.
OK, I added the fix and some additional LIT tests in #121206. I added more explanations there.
@nikic If isSignedMinMaxIntrinsicClamp returned true, the range is valid, i.e., *CLow <= *CRight. Thus, *CLow < *CRight + 1, except when *CRight is the max signed value. In such a case *CRight + 1 = min signed value and this is still ok if *CLow is larger than min signed value (the range is a valid interval from *CLow to max signed value).
The problem is when CLow->isMinSignedValue() && CHigh->isMaxSignedValue(). The constructor of ConstantRange has the asserion:
assert((Lower != Upper || (Lower.isMaxValue() || Lower.isMinValue())) &&
"Lower == Upper, but they aren't min or max value!");
with Lower.isMinValue() and not Lower.isMinSignedValue(). And so it fails.
A signed min-max clamp is the sequence of smin and smax intrinsics, which constrain a signed value into the range: smin <= value <= smax. The patch improves the calculation of KnownBits for a value subjected to the signed clamping.