Skip to content

Update GetRangeFromAssertions to handle some basic TYP_LONG scenarios where it FitsIn<int32_t> #128906

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

Open
tannergooding wants to merge 3 commits into
base: main
Choose a base branch
from
+342 −169
Open

Update GetRangeFromAssertions to handle some basic TYP_LONG scenarios where it FitsIn<int32_t> #128906

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f72c56d
Update GetRangeFromAssertions to handle some basic TYP_LONG scenarios…
tannergooding Jun 2, 2026
492a936
Don't change to the more expensive budget when having ComputeRange ca…
tannergooding Jun 4, 2026
707aa55
Handle some of the PR feedback, namely ensuring we validate the resul…
tannergooding Jun 4, 2026
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
152 changes: 82 additions & 70 deletions src/coreclr/jit/assertionprop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,10 @@ bool IntegralRange::Contains(int64_t value) const
{
rangeType = TYP_USHORT;
}
else if ((elementCount == 32) && varTypeIsLong(rangeType))
{
return {SymbolicIntegerValue::Zero, UpperBoundForType(TYP_UINT)};
Copy link
Copy Markdown
Member

@EgorBo EgorBo Jun 4, 2026

Choose a reason for hiding this comment

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

why not just rangeType = TYP_UINT like above?

Copy link
Copy Markdown
Member Author

@tannergooding tannergooding Jun 4, 2026

Choose a reason for hiding this comment

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

Because LowerBoundForType doesn't handle TYP_UINT, only UpperBoundForType does, and so ForType would hit an unreached.

I believe this is intentional and to avoid bugs since we shouldn't normally encounter TYP_UINT for anything except rare special scenarios like this

}
break;
}

Expand Down Expand Up @@ -4070,17 +4074,20 @@ void Compiler::optAssertionProp_RangeProperties(ASSERT_VALARG_TP assertions,
}

// Let's see if MergeEdgeAssertions can help us:
if (genActualType(tree) == TYP_INT)
if (varTypeIsIntegral(tree))
{
Range rng = RangeCheck::GetRangeFromAssertions(this, treeVN, assertions);
assert(rng.IsConstantRange());
if (rng.LowerLimit().GetConstant() >= 0)
{
*isKnownNonNegative = true;
}
if ((rng.LowerLimit().GetConstant() > 0) || (rng.UpperLimit().GetConstant() < 0))
Range rng = RangeCheck::GetRangeFromAssertions(this, tree->TypeGet(), treeVN, assertions);
Copy link
Copy Markdown
Member

@EgorBo EgorBo Jun 4, 2026

Choose a reason for hiding this comment

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

I really don't like the fact we need to pass type. It should be evaluated from VN, shouldn't it?

Copy link
Copy Markdown
Member Author

@tannergooding tannergooding Jun 4, 2026

Choose a reason for hiding this comment

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

The issue is the initial if ((num == ValueNumStore::NoVN) || (budget <= 0)) check in GetRangeFromAssertions.

i.e, handling producing a range if no VN exists, which previously relied on the fact we would only ever have TYP_INT, but now we can have TYP_LONG as well.

We'd have to have no VN produce keUnknown and for the callers to assume a constant range based on the type in that case instead, which is additional churn either way. I don't have a strong preference here on either approach, and went with passing the type through to avoid regressing the status quo.

Copy link
Copy Markdown
Member

@EgorBo EgorBo Jun 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

so can we return just keUnknown if no VN? since this function already may return keUnknown

Copy link
Copy Markdown
Member Author

@tannergooding tannergooding Jun 4, 2026

Choose a reason for hiding this comment

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

We could, but then that will regress TYP_INT scenarios with no VN where we previously would've gotten an appropriate [INT32_MIN, INT32_MAX] constant range.

That might be fine, since most things should have VNs, but it might also not be since we lose VN info in various places or may not have it for new nodes.

I could restrict it to just GetRangeFromAssertions, have it take the tree, extract the VN, and then call GetRangeFromAssertionsWorker which doesn't further propagate the type since it must be from a VN at that point, but I think that's the "best" we can do since we need a range for two possible types now.


if (rng.IsConstantRange())
Copy link
Copy Markdown
Member

@EgorBo EgorBo Jun 4, 2026

Choose a reason for hiding this comment

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

it's unfortunate we broke the contract for GetRangeFromAssertions to always return a constant range. My opinion we shouldn't do it and instead either upgrade Range to TYP_LONG or introduce a new GetRangeFromAssertions for 64-bit ranges

Copy link
Copy Markdown
Member Author

@tannergooding tannergooding Jun 4, 2026

Choose a reason for hiding this comment

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

or introduce a new GetRangeFromAssertions for 64-bit ranges

This is a lot of unnecessary code duplication and keeping the same overall handling regardless. That is, it doesn't change what assertionprop has to handle here, rather it just forces it to two paths one calling GetRangeFromAssertions32 and one calling GetRangeFromAssertions64 and still handling the fact that one path may not produce a constant range. It saves nothing and just makes things more complex.

With this approach, we have a single method handling both and the caller just has to handle the fact it might not be constant, rather than having to own dispatch to the right method and still handle that nuance anyways.

instead either upgrade Range to TYP_LONG

This is the eventual goal, but I'm trying to get this done incrementally and in a way that is easier to test, review, and handle.

Fully handling TYP_LONG is quite a bit more complex than only extending it to handle places where FitsIn<int32_t> remains "trivially true".

Namely, it involves extending Range/Limit to track int64_t cns, to then have all RangeOperations support int64_t, to have additional range ops that handle the 32 vs 64-bit limits, to ensure we understand ADD(int, x, y) and ADD(long, x, y) have different overflow limits to check and track for example, and to have the various handlers consider those nuances as well.

{
*isKnownNonZero = true;
if (rng.LowerLimit().GetConstant() >= 0)
{
*isKnownNonNegative = true;
}
if ((rng.LowerLimit().GetConstant() > 0) || (rng.UpperLimit().GetConstant() < 0))
{
*isKnownNonZero = true;
}
}
}
}
Expand All @@ -4101,14 +4108,17 @@ GenTree* Compiler::optAssertionProp_AddMulSub(ASSERT_VALARG_TP assertions, GenTr
{
assert(tree->OperIs(GT_MUL, GT_ADD, GT_SUB));

if (!optLocalAssertionProp && tree->TypeIs(TYP_INT) && tree->gtOverflow())
if (!optLocalAssertionProp && varTypeIsIntegral(tree) && tree->gtOverflow())
{
GenTree* op1 = tree->gtGetOp1();
GenTree* op2 = tree->gtGetOp2();

Range op1Rng = RangeCheck::GetRangeFromAssertions(this, optConservativeNormalVN(op1), assertions);
Range op2Rng = RangeCheck::GetRangeFromAssertions(this, optConservativeNormalVN(op2), assertions);
Range op1Rng =
RangeCheck::GetRangeFromAssertions(this, op1->TypeGet(), optConservativeNormalVN(op1), assertions);
Range op2Rng =
RangeCheck::GetRangeFromAssertions(this, op2->TypeGet(), optConservativeNormalVN(op2), assertions);
Range result = Limit(Limit::keUnknown);

if (tree->OperIs(GT_MUL))
{
result = RangeOps::Multiply(op1Rng, op2Rng, tree->IsUnsigned());
Expand Down Expand Up @@ -4481,10 +4491,10 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions,
}

// See if we can fold the relop based on range information.
// We don't need the op1->TypeIs(TYP_INT) check, but it seems to improve the TP quite a bit.
if (op1->TypeIs(TYP_INT))
// We don't need the varTypeIsIntegral(op1) check, but it seems to improve the TP quite a bit.
if (varTypeIsIntegral(op1))
{
Range relopRange = RangeCheck::GetRangeFromAssertions(this, relopVN, assertions);
Range relopRange = RangeCheck::GetRangeFromAssertions(this, tree->TypeGet(), relopVN, assertions);

int relopResult;
if (!relopRange.IsSingleValueConstant(&relopResult))
Expand All @@ -4500,8 +4510,8 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions,
}
else
{
Range op1Range = RangeCheck::GetRangeFromAssertions(this, op1VN, assertions);
Range op2Range = RangeCheck::GetRangeFromAssertions(this, op2VN, assertions);
Range op1Range = RangeCheck::GetRangeFromAssertions(this, op1->TypeGet(), op1VN, assertions);
Range op2Range = RangeCheck::GetRangeFromAssertions(this, op2->TypeGet(), op2VN, assertions);
relopRange = RangeOps::EvalRelop(tree->OperGet(), tree->IsUnsigned(), op1Range, op2Range);
}
}
Expand Down Expand Up @@ -4928,29 +4938,33 @@ GenTree* Compiler::optAssertionProp_Cast(ASSERT_VALARG_TP assertions,
if (FitsIn<int>(castLo) && FitsIn<int>(castHi))
{
Range castToTypeRange = Range(Limit(Limit::keConstant, (int)castLo), Limit(Limit::keConstant, (int)castHi));
if (castToTypeRange.IsConstantRange() && (genActualType(cast->CastOp()) == TYP_INT))
{
ValueNum castOpVN = optConservativeNormalVN(cast->CastOp());
Range castOpRng = RangeCheck::GetRangeFromAssertions(this, castOpVN, assertions);
assert(castOpRng.IsConstantRange());

int castFromLo = castOpRng.LowerLimit().GetConstant();
int castFromHi = castOpRng.UpperLimit().GetConstant();
int castToLo = castToTypeRange.LowerLimit().GetConstant();
int castToHi = castToTypeRange.UpperLimit().GetConstant();
GenTree* castOp = cast->CastOp();
if (castToTypeRange.IsConstantRange() && varTypeIsIntegral(castOp))
{
ValueNum castOpVN = optConservativeNormalVN(castOp);
Range castOpRng = RangeCheck::GetRangeFromAssertions(this, castOp->TypeGet(), castOpVN, assertions);

if ((castFromLo >= castToLo) && (castFromHi <= castToHi))
if (castOpRng.IsConstantRange())
{
if (canDropCast)
int castFromLo = castOpRng.LowerLimit().GetConstant();
int castFromHi = castOpRng.UpperLimit().GetConstant();
int castToLo = castToTypeRange.LowerLimit().GetConstant();
int castToHi = castToTypeRange.UpperLimit().GetConstant();

if ((castFromLo >= castToLo) && (castFromHi <= castToHi))
{
JITDUMP("Removing cast %06u as redundant based on VN assertions.\n", dspTreeID(cast));
return optAssertionProp_Update(cast->CastOp(), cast, stmt);
}
if (canDropCast)
{
JITDUMP("Removing cast %06u as redundant based on VN assertions.\n", dspTreeID(cast));
return optAssertionProp_Update(cast->CastOp(), cast, stmt);
}

assert(cast->gtOverflow());
JITDUMP("Clearing overflow flag for cast %06u based on VN assertions.\n", dspTreeID(cast));
cast->ClearOverflow();
return optAssertionProp_Update(cast, cast, stmt);
assert(cast->gtOverflow());
JITDUMP("Clearing overflow flag for cast %06u based on VN assertions.\n", dspTreeID(cast));
cast->ClearOverflow();
return optAssertionProp_Update(cast, cast, stmt);
}
}
}
}
Expand Down Expand Up @@ -5519,9 +5533,11 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, GenTree
}
#endif // FEATURE_ENABLE_NO_RANGE_CHECKS

GenTreeBoundsChk* arrBndsChk = tree->AsBoundsChk();
ValueNum vnCurIdx = vnStore->VNConservativeNormalValue(arrBndsChk->GetIndex()->gtVNPair);
ValueNum vnCurLen = vnStore->VNConservativeNormalValue(arrBndsChk->GetArrayLength()->gtVNPair);
GenTreeBoundsChk* arrBndsChk = tree->AsBoundsChk();
GenTree* arrBndsChkIdx = arrBndsChk->GetIndex();
GenTree* arrBndsChkLen = arrBndsChk->GetArrayLength();
ValueNum vnCurIdx = vnStore->VNConservativeNormalValue(arrBndsChk->GetIndex()->gtVNPair);
ValueNum vnCurLen = vnStore->VNConservativeNormalValue(arrBndsChk->GetArrayLength()->gtVNPair);
Comment on lines +5543 to +5547

auto dropBoundsCheck = [&](INDEBUG(const char* reason)) -> GenTree* {
JITDUMP("\nVN based redundant (%s) bounds check assertion prop in " FMT_BB ":\n", reason, compCurBB->bbNum);
Expand Down Expand Up @@ -5549,7 +5565,7 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, GenTree

if ((add0 == vnCurLen) && vnStore->IsVNInt32Constant(add1))
{
Range rng = RangeCheck::GetRangeFromAssertions(this, vnCurLen, assertions);
Range rng = RangeCheck::GetRangeFromAssertions(this, arrBndsChkLen->TypeGet(), vnCurLen, assertions);
// Lower known limit of ArrLen:
const int lenLowerLimit = rng.LowerLimit().GetConstant();
Comment thread
tannergooding marked this conversation as resolved.
Outdated

Expand Down Expand Up @@ -5582,7 +5598,7 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, GenTree
assert(curAssertion.GetOp2().IsVNNeverNegative());

// Do we have a previous range check involving the same 'vnLen' upper bound?
if (curAssertion.GetOp2().GetVN() == optConservativeNormalVN(arrBndsChk->GetArrayLength()))
if (curAssertion.GetOp2().GetVN() == optConservativeNormalVN(arrBndsChkLen))
{
// Do we have the exact same lower bound 'vnIdx'?
// a[i] followed by a[i]
Expand All @@ -5593,7 +5609,7 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, GenTree
// Are we using zero as the index?
// It can always be considered as redundant with any previous value
// a[*] followed by a[0]
else if (vnCurIdx == vnStore->VNZeroForType(arrBndsChk->GetIndex()->TypeGet()))
else if (vnCurIdx == vnStore->VNZeroForType(arrBndsChkIdx->TypeGet()))
{
return dropBoundsCheck(INDEBUG("a[*] followed by a[0]"));
}
Expand Down Expand Up @@ -5645,44 +5661,40 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, GenTree
}

// Let's see if we can remove the bounds check based on the ranges.
if ((genActualType(vnStore->TypeOfVN(vnCurIdx)) == TYP_INT) &&
(genActualType(vnStore->TypeOfVN(vnCurLen)) == TYP_INT))
Range idxRng = RangeCheck::GetRangeFromAssertions(this, arrBndsChkIdx->TypeGet(), vnCurIdx, assertions);
Range lenRng = RangeCheck::GetRangeFromAssertions(this, arrBndsChkLen->TypeGet(), vnCurLen, assertions);

if (idxRng.IsConstantRange() && lenRng.IsConstantRange())
{
Range idxRng = RangeCheck::GetRangeFromAssertions(this, vnCurIdx, assertions);
Range lenRng = RangeCheck::GetRangeFromAssertions(this, vnCurLen, assertions);
if (idxRng.IsConstantRange() && lenRng.IsConstantRange())
{
int idxLo = idxRng.LowerLimit().GetConstant();
int idxHi = idxRng.UpperLimit().GetConstant();
int lenLo = lenRng.LowerLimit().GetConstant();
int idxLo = idxRng.LowerLimit().GetConstant();
int idxHi = idxRng.UpperLimit().GetConstant();
int lenLo = lenRng.LowerLimit().GetConstant();

// GT_BOUNDS_CHECK node has an implicit contract - the length node must always be non-negative.
// So we additionally tighten the lower bound of lenLo to be ">= 1" when we also have a
// "length != 0" assertion for it.
if ((idxLo == 0) && (idxHi == 0) && (lenLo <= 0))
// GT_BOUNDS_CHECK node has an implicit contract - the length node must always be non-negative.
// So we additionally tighten the lower bound of lenLo to be ">= 1" when we also have a
// "length != 0" assertion for it.
if ((idxLo == 0) && (idxHi == 0) && (lenLo <= 0))
{
BitVecOps::Iter iter(apTraits, assertions);
unsigned bvIndex = 0;
while (iter.NextElem(&bvIndex))
{
BitVecOps::Iter iter(apTraits, assertions);
unsigned bvIndex = 0;
while (iter.NextElem(&bvIndex))
const AssertionDsc& assertion = optGetAssertion(GetAssertionIndex(bvIndex));
if (assertion.IsConstantInt32Assertion() && assertion.KindIs(OAK_NOT_EQUAL) &&
(assertion.GetOp1().GetVN() == vnCurLen) && (assertion.GetOp2().GetIntConstant() == 0))
{
const AssertionDsc& assertion = optGetAssertion(GetAssertionIndex(bvIndex));
if (assertion.IsConstantInt32Assertion() && assertion.KindIs(OAK_NOT_EQUAL) &&
(assertion.GetOp1().GetVN() == vnCurLen) && (assertion.GetOp2().GetIntConstant() == 0))
{
lenLo = 1;
break;
}
lenLo = 1;
break;
}
}
}

// index is always within [0..lenLo) --> drop bounds check
if ((idxLo >= 0) && (idxHi < lenLo))
{
return dropBoundsCheck(INDEBUG("upper bound of index is less than lower bound of length"));
}
// index is always within [0..lenLo) --> drop bounds check
if ((idxLo >= 0) && (idxHi < lenLo))
{
return dropBoundsCheck(INDEBUG("upper bound of index is less than lower bound of length"));
}
}

return nullptr;
}

Expand Down
Loading
Loading