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

Jit: Remove bounds checks with tests against length. #40180

Merged
merged 20 commits into from
Oct 6, 2020
Merged

Jit: Remove bounds checks with tests against length. #40180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9c4c520
Introduce a concept of minimum array length into range check
nathan-moore Aug 16, 2020
bce02ac
Some cleanup
nathan-moore Aug 22, 2020
45caddb
fix potential underflow
nathan-moore Aug 22, 2020
5e4640f
Merge branch 'master' into morphToUnsigned
nathan-moore Aug 24, 2020
aadeabe
bug fix and cleanup
nathan-moore Aug 27, 2020
ee54472
Merge branch 'master' into morphToUnsigned
nathan-moore Aug 27, 2020
6f77bf8
Revert string changes
nathan-moore Aug 28, 2020
49ce937
Merge branch 'master' into morphToUnsigned
nathan-moore Sep 3, 2020
0ec7bea
Allow elimination of arr[0] with len != 0 test
nathan-moore Aug 28, 2020
cd13bfc
Revert "Revert string changes"
nathan-moore Sep 4, 2020
4e0e9f7
Fix up tests
nathan-moore Sep 4, 2020
bbea251
reverting lower bound merging as it may be unsound
nathan-moore Sep 6, 2020
e750aa9
Fix CI exposed bug and add a couple of test cases
nathan-moore Sep 13, 2020
f7905df
code review feedback
nathan-moore Sep 15, 2020
2c422f7
Merge branch 'master' into morphToUnsigned
nathan-moore Sep 15, 2020
357a107
comment nit
nathan-moore Sep 15, 2020
57ad7ef
Merge branch 'master' into morphToUnsigned
nathan-moore Sep 29, 2020
cac0180
feedback
nathan-moore Oct 5, 2020
8afbc53
Merge branch 'master' into morphToUnsigned
nathan-moore Oct 5, 2020
1ee8eda
Add missing break
nathan-moore Oct 6, 2020
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
47 changes: 46 additions & 1 deletion src/coreclr/src/jit/assertionprop.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment should read:

// Is the current assertion a valid constant int32 assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the pre-existing style for this else if chain. I can change them over, but I personally like having the form there.

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