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
Merged

Use rangecheck in assertprop #112766

Show file tree
Hide file tree
Changes from 3 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
66 changes: 60 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,59 @@ 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))
{
auto getLowerBound = [&](ValueNum vn) -> Limit {
Range rng = Range(Limit(Limit::keDependent));
RangeCheck::MergeEdgeAssertions(this, vn, ValueNumStore::NoVN, assertions, &rng, false);
return rng.LowerLimit();
};

// See if (X + CNS) is known to be non-negative
if (tree->OperIs(GT_ADD) && tree->gtGetOp2()->IsIntCnsFitsInI32())
{
int cns = static_cast<int>(tree->gtGetOp2()->AsIntCon()->IconValue());
if ((cns < 0) && (cns > INT32_MIN))
{
Limit lowerBound = getLowerBound(vnStore->VNConservativeNormalValue(tree->gtGetOp1()->gtVNPair));
if (lowerBound.IsConstant())
{
// Say we have ADD(X, -8) and X is known to be [8..MAX_VALUE]
int lower = lowerBound.GetConstant();
if (lower >= -cns)
{
*isKnownNonNegative = true;
}
if (lower > -cns)
{
*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
{
Limit lowerBound = getLowerBound(treeVN);
if (lowerBound.IsConstant())
{
if (lowerBound.GetConstant() >= 0)
{
*isKnownNonNegative = true;
}
if (lowerBound.GetConstant() > 0)
{
*isKnownNonZero = true;
}
}
}
}
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -4851,12 +4905,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 +4918,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
82 changes: 52 additions & 30 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 @@ -664,13 +672,13 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse
}

// Walk through the "assertions" to check if the apply.
BitVecOps::Iter iter(m_pCompiler->apTraits, assertions);
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