Skip to content

Commit

Permalink
Jit: Remove bounds checks with tests against length. (#40180)
Browse files Browse the repository at this point in the history 
* Introduce a concept of minimum array length into range check

* Some cleanup

* fix potential underflow

* bug fix and cleanup

* Revert string changes

* Allow elimination of arr[0] with  len != 0 test

* Revert "Revert string changes"

This reverts commit 6f77bf8.

* Fix up tests

* reverting lower bound merging as it may be unsound

* Fix CI exposed bug and add a couple of test cases

* code review feedback

* comment nit

* feedback

* Add missing break
  • Loading branch information
@nathan-moore
nathan-moore authored Oct 6, 2020
1 parent d1c0fa8 commit 30e643e
Show file tree
Hide file tree
Showing 7 changed files with 699 additions and 56 deletions.
47 changes: 46 additions & 1 deletion src/coreclr/src/jit/assertionprop.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1625,7 +1625,6 @@ void Compiler::optDebugCheckAssertion(AssertionDsc* assertion)
assert(assertion->op2.u1.iconFlags != 0);
break;
case O1K_LCLVAR:
case O1K_ARR_BND:
assert((lvaTable[assertion->op1.lcl.lclNum].lvType != TYP_REF) || (assertion->op2.u1.iconVal == 0));
break;
case O1K_VALUE_NUMBER:
Expand Down Expand Up @@ -1959,12 +1958,58 @@ AssertionInfo Compiler::optAssertionGenJtrue(GenTree* tree)
{
std::swap(op1, op2);
}

ValueNum op1VN = vnStore->VNConservativeNormalValue(op1->gtVNPair);
ValueNum op2VN = vnStore->VNConservativeNormalValue(op2->gtVNPair);
// If op1 is lcl and op2 is const or lcl, create assertion.
if ((op1->gtOper == GT_LCL_VAR) &&
((op2->OperKind() & GTK_CONST) || (op2->gtOper == GT_LCL_VAR))) // Fix for Dev10 851483
{
return optCreateJtrueAssertions(op1, op2, assertionKind);
}
else if (vnStore->IsVNCheckedBound(op1VN) && vnStore->IsVNInt32Constant(op2VN))
{
assert(relop->OperIs(GT_EQ, GT_NE));

int con = vnStore->ConstantValue<int>(op2VN);
if (con >= 0)
{
AssertionDsc dsc;

// For arr.Length != 0, we know that 0 is a valid index
// For arr.Length == con, we know that con - 1 is the greatest valid index
if (con == 0)
{
dsc.assertionKind = OAK_NOT_EQUAL;
dsc.op1.bnd.vnIdx = vnStore->VNForIntCon(0);
}
else
{
dsc.assertionKind = OAK_EQUAL;
dsc.op1.bnd.vnIdx = vnStore->VNForIntCon(con - 1);
}

dsc.op1.vn = op1VN;
dsc.op1.kind = O1K_ARR_BND;
dsc.op1.bnd.vnLen = op1VN;
dsc.op2.vn = vnStore->VNConservativeNormalValue(op2->gtVNPair);
dsc.op2.kind = O2K_CONST_INT;
dsc.op2.u1.iconFlags = 0;
dsc.op2.u1.iconVal = 0;

// when con is not zero, create an assertion on the arr.Length == con edge
// when con is zero, create an assertion on the arr.Length != 0 edge
AssertionIndex index = optAddAssertion(&dsc);
if (relop->OperIs(GT_NE) != (con == 0))
{
return AssertionInfo::ForNextEdge(index);
}
else
{
return index;
}
}
}

// Check op1 and op2 for an indirection of a GT_LCL_VAR and keep it in op1.
if (((op1->gtOper != GT_IND) || (op1->AsOp()->gtOp1->gtOper != GT_LCL_VAR)) &&
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/src/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6912,6 +6912,11 @@ class Compiler
return ((assertionKind == OAK_EQUAL) && (op1.kind == O1K_LCLVAR) && (op2.kind == O2K_LCLVAR_COPY));
}

bool IsConstantInt32Assertion()
{
return ((assertionKind == OAK_EQUAL) || (assertionKind == OAK_NOT_EQUAL)) && (op2.kind == O2K_CONST_INT);
}

static bool SameKind(AssertionDsc* a1, AssertionDsc* a2)
{
return a1->assertionKind == a2->assertionKind && a1->op1.kind == a2->op1.kind &&
Expand Down
188 changes: 140 additions & 48 deletions src/coreclr/src/jit/rangecheck.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,29 @@ int RangeCheck::GetArrLength(ValueNum vn)
return m_pCompiler->vnStore->GetNewArrSize(arrRefVN);
}

// Check if the computed range is within bounds.
bool RangeCheck::BetweenBounds(Range& range, int lower, GenTree* upper)
//------------------------------------------------------------------------
// BetweenBounds: Check if the computed range is within bounds
//
// Arguments:
// Range - the range to check if in bounds
// upper - the array length vn
// arrSize - the length of the array if known, or <= 0
//
// Return Value:
// True iff range is between [0 and vn - 1] or [0, arrSize - 1]
//
// notes:
// This function assumes that the lower range is resolved and upper range is symbolic as in an
// increasing loop.
//
// TODO-CQ: This is not general enough.
//
bool RangeCheck::BetweenBounds(Range& range, GenTree* upper, int arrSize)
{
#ifdef DEBUG
if (m_pCompiler->verbose)
{
printf("%s BetweenBounds <%d, ", range.ToString(m_pCompiler->getAllocatorDebugOnly()), lower);
printf("%s BetweenBounds <%d, ", range.ToString(m_pCompiler->getAllocatorDebugOnly()), 0);
Compiler::printTreeID(upper);
printf(">\n");
}
Expand All @@ -85,28 +101,9 @@ bool RangeCheck::BetweenBounds(Range& range, int lower, GenTree* upper)
JITDUMP("\n");
#endif

int arrSize = 0;

if (vnStore->IsVNConstant(uLimitVN))
if ((arrSize <= 0) && !vnStore->IsVNCheckedBound(uLimitVN))
{
ssize_t constVal = -1;
unsigned iconFlags = 0;

if (m_pCompiler->optIsTreeKnownIntValue(true, upper, &constVal, &iconFlags))
{
arrSize = (int)constVal;
}
}
else if (vnStore->IsVNArrLen(uLimitVN))
{
// Get the array reference from the length.
ValueNum arrRefVN = vnStore->GetArrForLenVn(uLimitVN);
// Check if array size can be obtained.
arrSize = vnStore->GetNewArrSize(arrRefVN);
}
else if (!vnStore->IsVNCheckedBound(uLimitVN))
{
// If the upper limit is not length, then bail.
// If we don't know the array size and the upper limit is not known, then bail.
return false;
}

Expand Down Expand Up @@ -231,6 +228,19 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
#endif // FEATURE_SIMD
{
arrSize = GetArrLength(arrLenVn);

// if we can't find the array length, see if there
// are any assertions about the array size we can use to get a minimum length
if (arrSize <= 0)
{
JITDUMP("Looking for array size assertions for: " FMT_VN "\n", arrLenVn);
Range arrLength = Range(Limit(Limit::keDependent));
MergeEdgeAssertions(arrLenVn, block->bbAssertionIn, &arrLength);
if (arrLength.lLimit.IsConstant())
{
arrSize = arrLength.lLimit.GetConstant();
}
}
}

JITDUMP("ArrSize for lengthVN:%03X = %d\n", arrLenVn, arrSize);
Expand Down Expand Up @@ -286,7 +296,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
}

// Is the range between the lower and upper bound values.
if (BetweenBounds(range, 0, bndsChk->gtArrLen))
if (BetweenBounds(range, bndsChk->gtArrLen, arrSize))
{
JITDUMP("[RangeCheck::OptimizeRangeCheck] Between bounds\n");
m_pCompiler->optRemoveRangeCheck(treeParent, stmt);
Expand Down Expand Up @@ -532,18 +542,51 @@ void RangeCheck::SetDef(UINT64 hash, Location* loc)
}
#endif

// Merge assertions on the edge flowing into the block about a variable.
//------------------------------------------------------------------------
// MergeEdgeAssertions: Merge assertions on the edge flowing into the block about a variable
//
// Arguments:
// GenTreeLclVarCommon - the variable to look for assertions for
// assertions - the assertions to use
// pRange - the range to tighten with assertions
//
void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP assertions, Range* pRange)
{
if (lcl->GetSsaNum() == SsaConfig::RESERVED_SSA_NUM)
{
return;
}

LclVarDsc* varDsc = m_pCompiler->lvaGetDesc(lcl);
if (varDsc->CanBeReplacedWithItsField(m_pCompiler))
{
varDsc = m_pCompiler->lvaGetDesc(varDsc->lvFieldLclStart);
}
LclSsaVarDsc* ssaData = varDsc->GetPerSsaData(lcl->GetSsaNum());
ValueNum normalLclVN = m_pCompiler->vnStore->VNConservativeNormalValue(ssaData->m_vnPair);
MergeEdgeAssertions(normalLclVN, 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
//
void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP assertions, Range* pRange)
{
if (BitVecOps::IsEmpty(m_pCompiler->apTraits, assertions))
{
return;
}

if (lcl->GetSsaNum() == SsaConfig::RESERVED_SSA_NUM)
if (normalLclVN == ValueNumStore::NoVN)
{
return;
}

// Walk through the "assertions" to check if the apply.
BitVecOps::Iter iter(m_pCompiler->apTraits, assertions);
unsigned index = 0;
Expand All @@ -554,15 +597,8 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP
Compiler::AssertionDsc* curAssertion = m_pCompiler->optGetAssertion(assertionIndex);

Limit limit(Limit::keUndef);
genTreeOps cmpOper = GT_NONE;

LclVarDsc* varDsc = m_pCompiler->lvaGetDesc(lcl);
if (varDsc->CanBeReplacedWithItsField(m_pCompiler))
{
varDsc = m_pCompiler->lvaGetDesc(varDsc->lvFieldLclStart);
}
LclSsaVarDsc* ssaData = varDsc->GetPerSsaData(lcl->GetSsaNum());
ValueNum normalLclVN = m_pCompiler->vnStore->VNConservativeNormalValue(ssaData->m_vnPair);
genTreeOps cmpOper = GT_NONE;
bool isConstantAssertion = false;

// Current assertion is of the form (i < len - cns) != 0
if (curAssertion->IsCheckedBoundArithBound())
Expand Down Expand Up @@ -602,13 +638,20 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP
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)
if (normalLclVN == info.cmpOp)
{
cmpOper = (genTreeOps)info.cmpOper;
limit = Limit(Limit::keBinOpArray, info.vnBound, 0);
}
else if (normalLclVN == info.vnBound)
{
cmpOper = GenTree::SwapRelop((genTreeOps)info.cmpOper);
limit = Limit(Limit::keBinOpArray, info.cmpOp, 0);
}
else
{
continue;
}

limit = Limit(Limit::keBinOpArray, info.vnBound, 0);
cmpOper = (genTreeOps)info.cmpOper;
}
// Current assertion is of the form (i < 100) != 0
else if (curAssertion->IsConstantBound())
Expand All @@ -627,6 +670,36 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP
limit = Limit(Limit::keConstant, info.constVal);
cmpOper = (genTreeOps)info.cmpOper;
}
// Current assertion is of the form i == 100
else if (curAssertion->IsConstantInt32Assertion())
{
if (curAssertion->op1.vn != normalLclVN)
{
continue;
}

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

if ((cnstLimit == 0) && (curAssertion->assertionKind == Compiler::OAK_NOT_EQUAL) &&
m_pCompiler->vnStore->IsVNCheckedBound(curAssertion->op1.vn))
{
// we have arr.Len != 0, so the length must be atleast one
limit = Limit(Limit::keConstant, 1);
cmpOper = GT_GE;
}
else if (curAssertion->assertionKind == Compiler::OAK_EQUAL)
{
limit = Limit(Limit::keConstant, cnstLimit);
cmpOper = GT_EQ;
}
else
{
// We have a != assertion, but it doesn't tell us much about the interval. So just skip it.
continue;
}

isConstantAssertion = true;
}
// Current assertion is not supported, ignore it
else
{
Expand All @@ -635,8 +708,8 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP

assert(limit.IsBinOpArray() || limit.IsConstant());

// Make sure the assertion is of the form != 0 or == 0.
if (curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT))
// Make sure the assertion is of the form != 0 or == 0 if it isn't a constant assertion.
if (!isConstantAssertion && (curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)))
{
continue;
}
Expand All @@ -647,6 +720,17 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP
}
#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))
{
Limit tempLimit = Limit(Limit::keConstant, m_pCompiler->vnStore->ConstantValue<int>(limit.vn));
if (tempLimit.AddConstant(limit.cns))
{
limit = tempLimit;
}
}

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

if (m_pCompiler->vnStore->IsVNConstant(arrLenVN))
Expand All @@ -663,22 +747,25 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP
// (i < length) != 0
// (i < 100) == 0
// (i < 100) != 0
// i == 100
//
// At this point, we have detected that op1.vn is (i < length) or (i < length + cns) or
// (i < 100) and the op2.vn is 0.
// At this point, we have detected that either op1.vn is (i < length) or (i < length + cns) or
// (i < 100) and the op2.vn is 0 or that op1.vn is i and op2.vn is a known constant.
//
// Now, let us check if we are == 0 (i.e., op1 assertion is false) or != 0 (op1 assertion
// is true.),
// is true.).
//
// If we have an assertion of the form == 0 (i.e., equals false), then reverse relop.
// If we have a non-constant assertion of the form == 0 (i.e., equals false), then reverse relop.
// The relop has to be reversed because we have: (i < length) is false which is the same
// as (i >= length).
if (curAssertion->assertionKind == Compiler::OAK_EQUAL)
if ((curAssertion->assertionKind == Compiler::OAK_EQUAL) && !isConstantAssertion)
{
cmpOper = GenTree::ReverseRelop(cmpOper);
}

// Bounds are inclusive, so add -1 for upper bound when "<". But make sure we won't overflow.
assert(cmpOper != GT_NONE);

// Bounds are inclusive, so add -1 for upper bound when "<". But make sure we won't underflow.
if (cmpOper == GT_LT && !limit.AddConstant(-1))
{
continue;
Expand Down Expand Up @@ -744,6 +831,11 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP
pRange->lLimit = limit;
break;

case GT_EQ:
pRange->uLimit = limit;
pRange->lLimit = limit;
break;

default:
// All other 'cmpOper' kinds leave lLimit/uLimit unchanged
break;
Expand Down
Loading

0 comments on commit 30e643e

Please sign in to comment.