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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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))
nathan-moore marked this conversation as resolved.
Show resolved Hide resolved
{
nathan-moore marked this conversation as resolved.
Show resolved Hide resolved
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);
nathan-moore marked this conversation as resolved.
Show resolved Hide resolved
}

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);
nathan-moore marked this conversation as resolved.
Show resolved Hide resolved
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
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
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;
nathan-moore marked this conversation as resolved.
Show resolved Hide resolved
}

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)
nathan-moore marked this conversation as resolved.
Show resolved Hide resolved
{
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)
nathan-moore marked this conversation as resolved.
Show resolved Hide resolved
{
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.
nathan-moore marked this conversation as resolved.
Show resolved Hide resolved
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