Skip to content
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

Use rangecheck in assertprop #112766

Merged
merged 5 commits into from
Feb 22, 2025
+129 −38
Merged

Use rangecheck in assertprop #112766

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
98e809e
Use rangecheck in assertprop
EgorBo Feb 20, 2025
597e2ad
Fix CI
EgorBo Feb 21, 2025
3010416
clean up
EgorBo Feb 21, 2025
c239189
Clean up
EgorBo Feb 21, 2025
08338d5
Check overflow
EgorBo Feb 22, 2025
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
76 changes: 70 additions & 6 deletions src/coreclr/jit/assertionprop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
*/

#include "jitpch.h"
#include "rangecheck.h"
#ifdef _MSC_VER
#pragma hdrstop
#endif
Expand Down Expand Up @@ -4125,6 +4126,69 @@ void Compiler::optAssertionProp_RangeProperties(ASSERT_VALARG_TP assertions,
}
}
}

if (*isKnownNonZero && *isKnownNonNegative)
{
return;
}

// Let's see if MergeEdgeAssertions can help us:
if (tree->TypeIs(TYP_INT))
{
// See if (X + CNS) is known to be non-negative
if (tree->OperIs(GT_ADD) && tree->gtGetOp2()->IsIntCnsFitsInI32())
{
Range rng = Range(Limit(Limit::keDependent));
ValueNum vn = vnStore->VNConservativeNormalValue(tree->gtGetOp1()->gtVNPair);
RangeCheck::MergeEdgeAssertions(this, vn, ValueNumStore::NoVN, assertions, &rng, false);

int cns = static_cast<int>(tree->gtGetOp2()->AsIntCon()->IconValue());
rng.LowerLimit().AddConstant(cns);

if ((rng.LowerLimit().IsConstant() && !rng.LowerLimit().AddConstant(cns)) ||
(rng.UpperLimit().IsConstant() && !rng.UpperLimit().AddConstant(cns)))
{
// Add cns to both bounds if they are constants. Make sure the addition doesn't overflow.
return;
Comment on lines +4148 to +4152
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ((rng.LowerLimit().IsConstant() && !rng.LowerLimit().AddConstant(cns)) ||
(rng.UpperLimit().IsConstant() && !rng.UpperLimit().AddConstant(cns)))
{
// Add cns to both bounds if they are constants. Make sure the addition doesn't overflow.
return;
if (!rng.LowerLimit().AddConstant(cns) || !rng.UpperLimit().AddConstant(cns))
{
// Add cns to both bounds. Make sure the addition doesn't overflow.
return;

Should be ok since there's the check for IsConstant below already?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. This is too conservative - we're fine with some bounds to be Dependent (non-constant) when we e.g. only check Lower one. AddConstant returns false for them making the improvement super conservative

}

if (rng.LowerLimit().IsConstant())
{
// E.g. "X + -8" when X's range is [8..unknown]
// it's safe to say "X + -8" is non-negative
if ((rng.LowerLimit().GetConstant() == 0))
{
*isKnownNonNegative = true;
}
Comment on lines +4159 to +4162
Copy link
Member

Choose a reason for hiding this comment

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

Does this check not need to account for overflow?

Copy link
Member Author

Choose a reason for hiding this comment

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

but how can X - CNS + CNS overflow?

Copy link
Member Author

Choose a reason for hiding this comment

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

(AddConstant checks for overflow before it)

Copy link
Member

Choose a reason for hiding this comment

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

I was worried that for a range like "[8..unknown]", that "unknown" could mean "some computation that overflowed" and hence that the lower limit was actually smaller than 8. But it's probably ok since the range here comes from assertions only.


// E.g. "X + 8" when X's range is [0..CNS]
// Here we have to check the upper bound as well to avoid overflow
if ((rng.LowerLimit().GetConstant() > 0) && rng.UpperLimit().IsConstant() &&
rng.UpperLimit().GetConstant() > rng.LowerLimit().GetConstant())
{
*isKnownNonNegative = true;
*isKnownNonZero = true;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, you had a version where you tried to use range check directly on the IR node. It seems like it would catch more cases, because the logic here for additions should already be conceptually the same logic that range check has to handle additions.

Copy link
Member Author

Choose a reason for hiding this comment

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

That version had terrible TP impact and needed more touches (perhaps, its cache can be shared with rangecheck?), I might investigate it again in the future

Copy link
Member Author

@EgorBo EgorBo Feb 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I'll do it after this PR - diffs look nice (if TP regressions are mitigated)

else
{
Range rng = Range(Limit(Limit::keDependent));
RangeCheck::MergeEdgeAssertions(this, treeVN, ValueNumStore::NoVN, assertions, &rng, false);
Limit lowerBound = rng.LowerLimit();
if (lowerBound.IsConstant())
{
if (lowerBound.GetConstant() >= 0)
{
*isKnownNonNegative = true;
}
if (lowerBound.GetConstant() > 0)
{
*isKnownNonZero = true;
}
}
}
Comment on lines +4174 to +4190
Copy link
Member

Choose a reason for hiding this comment

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

Does this part catch cases? Naively I would expect assertion prop to know to check for the "straightforward" assertions that range check is checking here already.

Copy link
Member Author

@EgorBo EgorBo Feb 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, good chunk of diffs are caught by this

Copy link
Member

@jakobbotsch jakobbotsch Feb 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if we could get rid of some of the checks above then, I would expect there to be a good amount of duplication between range check's assertion handling and that assertion handling above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, I'll check what is missing in the above

}
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -4851,12 +4915,6 @@ GenTree* Compiler::optAssertionProp_Cast(ASSERT_VALARG_TP assertions, GenTreeCas
// Skip over a GT_COMMA node(s), if necessary to get to the lcl.
GenTree* lcl = op1->gtEffectiveVal();

// If we don't have a cast of a LCL_VAR then bail.
if (!lcl->OperIs(GT_LCL_VAR))
{
return nullptr;
}

// Try and see if we can make this cast into a cheaper zero-extending version
// if the input is known to be non-negative.
if (!cast->IsUnsigned() && genActualTypeIsInt(lcl) && cast->TypeIs(TYP_LONG) && (TARGET_POINTER_SIZE == 8))
Expand All @@ -4870,6 +4928,12 @@ GenTree* Compiler::optAssertionProp_Cast(ASSERT_VALARG_TP assertions, GenTreeCas
}
}

// If we don't have a cast of a LCL_VAR then bail.
if (!lcl->OperIs(GT_LCL_VAR))
{
return nullptr;
}

IntegralRange range = IntegralRange::ForCastInput(cast);
AssertionIndex index = optAssertionIsSubrange(lcl, range, assertions);
if (index != NO_ASSERTION_INDEX)
Expand Down
84 changes: 53 additions & 31 deletions src/coreclr/jit/rangecheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
{
JITDUMP("Looking for array size assertions for: " FMT_VN "\n", arrLenVn);
Range arrLength = Range(Limit(Limit::keDependent));
MergeEdgeAssertions(arrLenVn, block->bbAssertionIn, &arrLength);
MergeEdgeAssertions(m_pCompiler, arrLenVn, arrLenVn, block->bbAssertionIn, &arrLength);
if (arrLength.lLimit.IsConstant())
{
arrSize = arrLength.lLimit.GetConstant();
Expand Down Expand Up @@ -640,20 +640,28 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP

LclSsaVarDsc* ssaData = m_pCompiler->lvaGetDesc(lcl)->GetPerSsaData(lcl->GetSsaNum());
ValueNum normalLclVN = m_pCompiler->vnStore->VNConservativeNormalValue(ssaData->m_vnPair);
MergeEdgeAssertions(normalLclVN, assertions, pRange);
ValueNum arrLenVN = m_pCompiler->vnStore->VNConservativeNormalValue(m_pCurBndsChk->GetArrayLength()->gtVNPair);
MergeEdgeAssertions(m_pCompiler, normalLclVN, arrLenVN, assertions, pRange);
}

//------------------------------------------------------------------------
// MergeEdgeAssertions: Merge assertions on the edge flowing into the block about a variable
//
// Arguments:
// normalLclVN - the value number to look for assertions for
// assertions - the assertions to use
// pRange - the range to tighten with assertions
// comp - the compiler instance
// normalLclVN - the value number to look for assertions for
// preferredBoundVN - when this VN is set, it will be given preference over constant limits
// assertions - the assertions to use
// pRange - the range to tighten with assertions
//
void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP assertions, Range* pRange)
void RangeCheck::MergeEdgeAssertions(Compiler* comp,
ValueNum normalLclVN,
ValueNum preferredBoundVN,
ASSERT_VALARG_TP assertions,
Range* pRange,
bool log)
{
if (BitVecOps::IsEmpty(m_pCompiler->apTraits, assertions))
if (BitVecOps::IsEmpty(comp->apTraits, assertions))
{
return;
}
Expand All @@ -663,14 +671,14 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse
return;
}

// Walk through the "assertions" to check if the apply.
BitVecOps::Iter iter(m_pCompiler->apTraits, assertions);
// Walk through the "assertions" to check if they apply.
BitVecOps::Iter iter(comp->apTraits, assertions);
unsigned index = 0;
while (iter.NextElem(&index))
{
AssertionIndex assertionIndex = GetAssertionIndex(index);

Compiler::AssertionDsc* curAssertion = m_pCompiler->optGetAssertion(assertionIndex);
Compiler::AssertionDsc* curAssertion = comp->optGetAssertion(assertionIndex);

Limit limit(Limit::keUndef);
genTreeOps cmpOper = GT_NONE;
Expand All @@ -683,7 +691,7 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse
ValueNumStore::CompareCheckedBoundArithInfo info;

// Get i, len, cns and < as "info."
m_pCompiler->vnStore->GetCompareCheckedBoundArithInfo(curAssertion->op1.vn, &info);
comp->vnStore->GetCompareCheckedBoundArithInfo(curAssertion->op1.vn, &info);

// If we don't have the same variable we are comparing against, bail.
if (normalLclVN != info.cmpOp)
Expand All @@ -697,12 +705,12 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse
}

// If the operand that operates on the bound is not constant, then done.
if (!m_pCompiler->vnStore->IsVNInt32Constant(info.arrOp))
if (!comp->vnStore->IsVNInt32Constant(info.arrOp))
{
continue;
}

int cons = m_pCompiler->vnStore->ConstantValue<int>(info.arrOp);
int cons = comp->vnStore->ConstantValue<int>(info.arrOp);
limit = Limit(Limit::keBinOpArray, info.vnBound, info.arrOper == GT_SUB ? -cons : cons);
cmpOper = (genTreeOps)info.cmpOper;
}
Expand All @@ -712,7 +720,7 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse
ValueNumStore::CompareCheckedBoundArithInfo info;

// Get the info as "i", "<" and "len"
m_pCompiler->vnStore->GetCompareCheckedBound(curAssertion->op1.vn, &info);
comp->vnStore->GetCompareCheckedBound(curAssertion->op1.vn, &info);

// If we don't have the same variable we are comparing against, bail.
if (normalLclVN == info.cmpOp)
Expand All @@ -736,7 +744,7 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse
ValueNumStore::ConstantBoundInfo info;

// Get the info as "i", "<" and "100"
m_pCompiler->vnStore->GetConstantBoundInfo(curAssertion->op1.vn, &info);
comp->vnStore->GetConstantBoundInfo(curAssertion->op1.vn, &info);

// If we don't have the same variable we are comparing against, bail.
if (normalLclVN != info.cmpOpVN)
Expand All @@ -756,10 +764,10 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse
continue;
}

int cnstLimit = m_pCompiler->vnStore->CoercedConstantValue<int>(curAssertion->op2.vn);
int cnstLimit = comp->vnStore->CoercedConstantValue<int>(curAssertion->op2.vn);

if ((cnstLimit == 0) && (curAssertion->assertionKind == Compiler::OAK_NOT_EQUAL) &&
m_pCompiler->vnStore->IsVNCheckedBound(curAssertion->op1.vn))
comp->vnStore->IsVNCheckedBound(curAssertion->op1.vn))
{
// we have arr.Len != 0, so the length must be atleast one
limit = Limit(Limit::keConstant, 1);
Expand Down Expand Up @@ -804,31 +812,31 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse

// Make sure the assertion is of the form != 0 or == 0 if it isn't a constant assertion.
if (!isConstantAssertion && (curAssertion->assertionKind != Compiler::OAK_NO_THROW) &&
(curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)))
(curAssertion->op2.vn != comp->vnStore->VNZeroForType(TYP_INT)))
{
continue;
}
#ifdef DEBUG
if (m_pCompiler->verbose)
if (comp->verbose)
{
m_pCompiler->optPrintAssertion(curAssertion, assertionIndex);
comp->optPrintAssertion(curAssertion, assertionIndex);
}
#endif

// Limits are sometimes made with the form vn + constant, where vn is a known constant
// see if we can simplify this to just a constant
if (limit.IsBinOpArray() && m_pCompiler->vnStore->IsVNInt32Constant(limit.vn))
if (limit.IsBinOpArray() && comp->vnStore->IsVNInt32Constant(limit.vn))
{
Limit tempLimit = Limit(Limit::keConstant, m_pCompiler->vnStore->ConstantValue<int>(limit.vn));
Limit tempLimit = Limit(Limit::keConstant, comp->vnStore->ConstantValue<int>(limit.vn));
if (tempLimit.AddConstant(limit.cns))
{
limit = tempLimit;
}
}

ValueNum arrLenVN = m_pCompiler->vnStore->VNConservativeNormalValue(m_pCurBndsChk->GetArrayLength()->gtVNPair);
ValueNum arrLenVN = preferredBoundVN;

if (m_pCompiler->vnStore->IsVNConstant(arrLenVN))
if (comp->vnStore->IsVNConstant(arrLenVN))
{
// Set arrLenVN to NoVN; this will make it match the "vn" recorded on
// constant limits (where we explicitly track the constant and don't
Expand Down Expand Up @@ -917,7 +925,10 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse

if (limit.vn != arrLenVN)
{
JITDUMP("Array length VN did not match arrLen=" FMT_VN ", limit=" FMT_VN "\n", arrLenVN, limit.vn);
if (log)
{
JITDUMP("Array length VN did not match arrLen=" FMT_VN ", limit=" FMT_VN "\n", arrLenVN, limit.vn);
}
continue;
}

Expand All @@ -927,7 +938,10 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse
// Incoming limit doesn't tighten the existing upper limit.
if (limCns >= curCns)
{
JITDUMP("Bound limit %d doesn't tighten current bound %d\n", limCns, curCns);
if (log)
{
JITDUMP("Bound limit %d doesn't tighten current bound %d\n", limCns, curCns);
}
continue;
}
}
Expand All @@ -954,8 +968,12 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse

case GT_GT:
case GT_GE:
pRange->lLimit = limit;
// it doesn't matter if it's isUnsigned or not here - it's not negative anyway.
// GT/GE being unsigned creates a non-contiguous range which we can't represent
// using single Range object.
if (!isUnsigned)
{
pRange->lLimit = limit;
}
break;

case GT_EQ:
Expand All @@ -967,9 +985,13 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse
// All other 'cmpOper' kinds leave lLimit/uLimit unchanged
break;
}
JITDUMP("The range after edge merging:");
JITDUMP(pRange->ToString(m_pCompiler));
JITDUMP("\n");

if (log)
{
JITDUMP("The range after edge merging:");
JITDUMP(pRange->ToString(comp));
JITDUMP("\n");
}
}
}

Expand Down
7 changes: 6 additions & 1 deletion src/coreclr/jit/rangecheck.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,12 @@ class RangeCheck
void MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP assertions, Range* pRange);

// Inspect the assertions about the current ValueNum to refine pRange
void MergeEdgeAssertions(ValueNum num, ASSERT_VALARG_TP assertions, Range* pRange);
static void MergeEdgeAssertions(Compiler* comp,
ValueNum num,
ValueNum preferredBoundVN,
ASSERT_VALARG_TP assertions,
Range* pRange,
bool log = true);

// The maximum possible value of the given "limit". If such a value could not be determined
// return "false". For example: CORINFO_Array_MaxLength for array length.
Expand Down
Loading