Skip to content

Commit

Permalink
Revert "Use rangecheck in assertprop (#112766)" (#112872)
Browse files Browse the repository at this point in the history 
This reverts commit 6022adf.
  • Loading branch information
@AndyAyersMS
AndyAyersMS authored Feb 24, 2025
1 parent 2a19c9d commit db681fb
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 129 deletions.
76 changes: 6 additions & 70 deletions src/coreclr/jit/assertionprop.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
*/

#include "jitpch.h"
#include "rangecheck.h"
#ifdef _MSC_VER
#pragma hdrstop
#endif
Expand Down Expand Up @@ -4126,69 +4125,6 @@ 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;
}

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;
}

// 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;
}
}
}
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;
}
}
}
}
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -4915,6 +4851,12 @@ 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 @@ -4928,12 +4870,6 @@ 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: 31 additions & 53 deletions src/coreclr/jit/rangecheck.cpp
View file
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(m_pCompiler, arrLenVn, arrLenVn, block->bbAssertionIn, &arrLength);
MergeEdgeAssertions(arrLenVn, block->bbAssertionIn, &arrLength);
if (arrLength.lLimit.IsConstant())
{
arrSize = arrLength.lLimit.GetConstant();
Expand Down Expand Up @@ -640,28 +640,20 @@ 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);
ValueNum arrLenVN = m_pCompiler->vnStore->VNConservativeNormalValue(m_pCurBndsChk->GetArrayLength()->gtVNPair);
MergeEdgeAssertions(m_pCompiler, normalLclVN, arrLenVN, assertions, pRange);
MergeEdgeAssertions(normalLclVN, assertions, pRange);
}

//------------------------------------------------------------------------
// MergeEdgeAssertions: Merge assertions on the edge flowing into the block about a variable
//
// Arguments:
// 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
// normalLclVN - the value number to look for assertions for
// assertions - the assertions to use
// pRange - the range to tighten with assertions
//
void RangeCheck::MergeEdgeAssertions(Compiler* comp,
ValueNum normalLclVN,
ValueNum preferredBoundVN,
ASSERT_VALARG_TP assertions,
Range* pRange,
bool log)
void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP assertions, Range* pRange)
{
if (BitVecOps::IsEmpty(comp->apTraits, assertions))
if (BitVecOps::IsEmpty(m_pCompiler->apTraits, assertions))
{
return;
}
Expand All @@ -671,14 +663,14 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp,
return;
}

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

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

Limit limit(Limit::keUndef);
genTreeOps cmpOper = GT_NONE;
Expand All @@ -691,7 +683,7 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp,
ValueNumStore::CompareCheckedBoundArithInfo info;

// Get i, len, cns and < as "info."
comp->vnStore->GetCompareCheckedBoundArithInfo(curAssertion->op1.vn, &info);
m_pCompiler->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 @@ -705,12 +697,12 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp,
}

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

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

// Get the info as "i", "<" and "len"
comp->vnStore->GetCompareCheckedBound(curAssertion->op1.vn, &info);
m_pCompiler->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 @@ -744,7 +736,7 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp,
ValueNumStore::ConstantBoundInfo info;

// Get the info as "i", "<" and "100"
comp->vnStore->GetConstantBoundInfo(curAssertion->op1.vn, &info);
m_pCompiler->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 @@ -764,10 +756,10 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp,
continue;
}

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

if ((cnstLimit == 0) && (curAssertion->assertionKind == Compiler::OAK_NOT_EQUAL) &&
comp->vnStore->IsVNCheckedBound(curAssertion->op1.vn))
m_pCompiler->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 @@ -812,31 +804,31 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp,

// 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 != comp->vnStore->VNZeroForType(TYP_INT)))
(curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)))
{
continue;
}
#ifdef DEBUG
if (comp->verbose)
if (m_pCompiler->verbose)
{
comp->optPrintAssertion(curAssertion, assertionIndex);
m_pCompiler->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() && comp->vnStore->IsVNInt32Constant(limit.vn))
if (limit.IsBinOpArray() && m_pCompiler->vnStore->IsVNInt32Constant(limit.vn))
{
Limit tempLimit = Limit(Limit::keConstant, comp->vnStore->ConstantValue<int>(limit.vn));
Limit tempLimit = Limit(Limit::keConstant, m_pCompiler->vnStore->ConstantValue<int>(limit.vn));
if (tempLimit.AddConstant(limit.cns))
{
limit = tempLimit;
}
}

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

if (comp->vnStore->IsVNConstant(arrLenVN))
if (m_pCompiler->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 @@ -925,10 +917,7 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp,

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

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

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

case GT_EQ:
Expand All @@ -985,13 +967,9 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp,
// All other 'cmpOper' kinds leave lLimit/uLimit unchanged
break;
}

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

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

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

// 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

0 comments on commit db681fb

Please sign in to comment.