Skip to content

[ValueTracking] Improve KnownBits for signed min-max clamping #120576

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Dec 25, 2024
Merged

[ValueTracking] Improve KnownBits for signed min-max clamping #120576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cb2efa6
[ValueTracking] Improve KnownBits for signed min-max clamping
adam-bzowski Dec 19, 2024
54b6643
[ValueTracking] Improve KnownBits for signed min-max clamping
adam-bzowski Dec 19, 2024
a72e960
[ValueTracking] Improve KnownBits for signed min-max clamping
adam-bzowski Dec 19, 2024
6636dc6
[ValueTracking] Improve KnownBits for signed min-max clamping
adam-bzowski Dec 20, 2024
5422719
[ValueTracking] Improve KnownBits for signed min-max clamping
adam-bzowski Dec 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 59 additions & 49 deletions llvm/lib/Analysis/ValueTracking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1065,6 +1065,63 @@ 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 + 1).toKnownBits());
Copy link
Collaborator

@topperc topperc Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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"

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch, this should be using ConstantRange::getNonEmpty().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the fix: #121206. I'm still playing with some tests. Thanks for the comment and sorry for the trouble.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

static void computeKnownBitsFromOperator(const Operator *I,
const APInt &DemandedElts,
KnownBits &Known, unsigned Depth,
Expand Down Expand Up @@ -1804,11 +1861,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);
Expand Down Expand Up @@ -3751,55 +3810,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
Expand Down
Loading
Loading