Skip to content

Commit

Permalink
JIT: Add support for bounds check no throw assertions in range check …
Browse files Browse the repository at this point in the history 
…and make overflow check more precise (dotnet#100777)

Fix dotnet#9422
Fix dotnet#83349
  • Loading branch information
@jakobbotsch @matouskozak
jakobbotsch authored and matouskozak committed Apr 30, 2024
1 parent e223770 commit cd0cd23
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 65 deletions.
4 changes: 2 additions & 2 deletions src/coreclr/jit/assertionprop.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -769,9 +769,9 @@ void Compiler::optPrintAssertion(AssertionDsc* curAssertion, AssertionIndex asse
}
else if (curAssertion->op1.kind == O1K_ARR_BND)
{
printf("[idx:");
printf("[idx: " FMT_VN, curAssertion->op1.bnd.vnIdx);
vnStore->vnDump(this, curAssertion->op1.bnd.vnIdx);
printf(";len:");
printf("; len: " FMT_VN, curAssertion->op1.bnd.vnLen);
vnStore->vnDump(this, curAssertion->op1.bnd.vnLen);
printf("]");
}
Expand Down
79 changes: 62 additions & 17 deletions src/coreclr/jit/rangecheck.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
return;
}

if (DoesOverflow(block, treeIndex))
if (DoesOverflow(block, treeIndex, range))
{
JITDUMP("Method determined to overflow.\n");
return;
Expand Down Expand Up @@ -773,6 +773,22 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse

isConstantAssertion = true;
}
// Current assertion asserts a bounds check does not throw
else if (curAssertion->IsBoundsCheckNoThrow())
{
ValueNum indexVN = curAssertion->op1.bnd.vnIdx;
ValueNum lenVN = curAssertion->op1.bnd.vnLen;
if (normalLclVN == indexVN)
{
isUnsigned = true;
cmpOper = GT_LT;
limit = Limit(Limit::keBinOpArray, lenVN, 0);
}
else
{
continue;
}
}
// Current assertion is not supported, ignore it
else
{
Expand All @@ -782,7 +798,8 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse
assert(limit.IsBinOpArray() || limit.IsConstant());

// 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)))
if (!isConstantAssertion && (curAssertion->assertionKind != Compiler::OAK_NO_THROW) &&
(curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)))
{
continue;
}
Expand Down Expand Up @@ -1235,17 +1252,17 @@ bool RangeCheck::MultiplyOverflows(Limit& limit1, Limit& limit2)
}

// Does the bin operation overflow.
bool RangeCheck::DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop)
bool RangeCheck::DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop, const Range& range)
{
GenTree* op1 = binop->gtGetOp1();
GenTree* op2 = binop->gtGetOp2();

if (!m_pSearchPath->Lookup(op1) && DoesOverflow(block, op1))
if (!m_pSearchPath->Lookup(op1) && DoesOverflow(block, op1, range))
{
return true;
}

if (!m_pSearchPath->Lookup(op2) && DoesOverflow(block, op2))
if (!m_pSearchPath->Lookup(op2) && DoesOverflow(block, op2, range))
{
return true;
}
Expand Down Expand Up @@ -1279,7 +1296,7 @@ bool RangeCheck::DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop)
}

// Check if the var definition the rhs involves arithmetic that overflows.
bool RangeCheck::DoesVarDefOverflow(GenTreeLclVarCommon* lcl)
bool RangeCheck::DoesVarDefOverflow(BasicBlock* block, GenTreeLclVarCommon* lcl, const Range& range)
{
LclSsaVarDsc* ssaDef = GetSsaDefStore(lcl);
if (ssaDef == nullptr)
Expand All @@ -1291,10 +1308,25 @@ bool RangeCheck::DoesVarDefOverflow(GenTreeLclVarCommon* lcl)
}
return true;
}
return DoesOverflow(ssaDef->GetBlock(), ssaDef->GetDefNode()->Data());

// We can use intermediate assertions about the local to prove that any
// overflow on this path does not matter for the range computed.
Range assertionRange = Range(Limit(Limit::keUnknown));
MergeAssertion(block, lcl, &assertionRange DEBUGARG(0));

// But only if the range from the assertion is more strict than the global
// range computed; otherwise we might still have used the def's value to
// tighten the range of the global range.
Range merged = RangeOps::Merge(range, assertionRange, false);
if (merged.LowerLimit().Equals(range.LowerLimit()) && merged.UpperLimit().Equals(range.UpperLimit()))
{
return false;
}

return DoesOverflow(ssaDef->GetBlock(), ssaDef->GetDefNode()->Data(), range);
}

bool RangeCheck::DoesPhiOverflow(BasicBlock* block, GenTree* expr)
bool RangeCheck::DoesPhiOverflow(BasicBlock* block, GenTree* expr, const Range& range)
{
for (GenTreePhi::Use& use : expr->AsPhi()->Uses())
{
Expand All @@ -1303,25 +1335,38 @@ bool RangeCheck::DoesPhiOverflow(BasicBlock* block, GenTree* expr)
{
continue;
}
if (DoesOverflow(block, arg))
if (DoesOverflow(block, arg, range))
{
return true;
}
}
return false;
}

bool RangeCheck::DoesOverflow(BasicBlock* block, GenTree* expr)
//------------------------------------------------------------------------
// DoesOverflow: Check if the computation of "expr" may have overflowed.
//
// Arguments:
// block - the block that contains `expr`
// expr - expression to check overflow of
// range - range that we believe "expr" to be in without accounting for
// overflow; used to ignore potential overflow on paths where
// we can prove the value is in this range regardless.
//
// Return value:
// True if the computation may have involved an impactful overflow.
//
bool RangeCheck::DoesOverflow(BasicBlock* block, GenTree* expr, const Range& range)
{
bool overflows = false;
if (!GetOverflowMap()->Lookup(expr, &overflows))
{
overflows = ComputeDoesOverflow(block, expr);
overflows = ComputeDoesOverflow(block, expr, range);
}
return overflows;
}

bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr)
bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr, const Range& range)
{
JITDUMP("Does overflow [%06d]?\n", Compiler::dspTreeID(expr));
m_pSearchPath->Set(expr, block, SearchPath::Overwrite);
Expand All @@ -1343,17 +1388,17 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr)
}
else if (expr->OperIs(GT_COMMA))
{
overflows = ComputeDoesOverflow(block, expr->gtEffectiveVal());
overflows = ComputeDoesOverflow(block, expr->gtEffectiveVal(), range);
}
// Check if the var def has rhs involving arithmetic that overflows.
else if (expr->IsLocal())
{
overflows = DoesVarDefOverflow(expr->AsLclVarCommon());
overflows = DoesVarDefOverflow(block, expr->AsLclVarCommon(), range);
}
// Check if add overflows.
else if (expr->OperIs(GT_ADD, GT_MUL))
{
overflows = DoesBinOpOverflow(block, expr->AsOp());
overflows = DoesBinOpOverflow(block, expr->AsOp(), range);
}
// These operators don't overflow.
// Actually, GT_LSH can overflow so it depends on the analysis done in ComputeRangeForBinOp
Expand All @@ -1364,11 +1409,11 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr)
// Walk through phi arguments to check if phi arguments involve arithmetic that overflows.
else if (expr->OperIs(GT_PHI))
{
overflows = DoesPhiOverflow(block, expr);
overflows = DoesPhiOverflow(block, expr, range);
}
else if (expr->OperIs(GT_CAST))
{
overflows = ComputeDoesOverflow(block, expr->gtGetOp1());
overflows = ComputeDoesOverflow(block, expr->gtGetOp1(), range);
}
GetOverflowMap()->Set(expr, overflows, OverflowMap::Overwrite);
m_pSearchPath->Remove(expr);
Expand Down
34 changes: 22 additions & 12 deletions src/coreclr/jit/rangecheck.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ struct Limit
return false;
}

bool Equals(Limit& l)
bool Equals(const Limit& l) const
{
switch (type)
{
Expand Down Expand Up @@ -262,11 +262,21 @@ struct Range
{
}

const Limit& UpperLimit() const
{
return uLimit;
}

Limit& UpperLimit()
{
return uLimit;
}

const Limit& LowerLimit() const
{
return lLimit;
}

Limit& LowerLimit()
{
return lLimit;
Expand Down Expand Up @@ -440,12 +450,12 @@ struct RangeOps

// Given two ranges "r1" and "r2", do a Phi merge. If "monIncreasing" is true,
// then ignore the dependent variables for the lower bound but not for the upper bound.
static Range Merge(Range& r1, Range& r2, bool monIncreasing)
static Range Merge(const Range& r1, const Range& r2, bool monIncreasing)
{
Limit& r1lo = r1.LowerLimit();
Limit& r1hi = r1.UpperLimit();
Limit& r2lo = r2.LowerLimit();
Limit& r2hi = r2.UpperLimit();
const Limit& r1lo = r1.LowerLimit();
const Limit& r1hi = r1.UpperLimit();
const Limit& r2lo = r2.LowerLimit();
const Limit& r2hi = r2.UpperLimit();

// Take care of lo part.
Range result = Limit(Limit::keUnknown);
Expand Down Expand Up @@ -689,19 +699,19 @@ class RangeCheck
bool MultiplyOverflows(Limit& limit1, Limit& limit2);

// Does the binary operation between the operands overflow? Check recursively.
bool DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop);
bool DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop, const Range& range);

// Do the phi operands involve a definition that could overflow?
bool DoesPhiOverflow(BasicBlock* block, GenTree* expr);
bool DoesPhiOverflow(BasicBlock* block, GenTree* expr, const Range& range);

// Find the def of the "expr" local and recurse on the arguments if any of them involve a
// calculation that overflows.
bool DoesVarDefOverflow(GenTreeLclVarCommon* lcl);
bool DoesVarDefOverflow(BasicBlock* block, GenTreeLclVarCommon* lcl, const Range& range);

bool ComputeDoesOverflow(BasicBlock* block, GenTree* expr);
bool ComputeDoesOverflow(BasicBlock* block, GenTree* expr, const Range& range);

// Does the current "expr" which is a use involve a definition, that overflows.
bool DoesOverflow(BasicBlock* block, GenTree* tree);
// Does the current "expr", which is a use, involve a definition that overflows.
bool DoesOverflow(BasicBlock* block, GenTree* tree, const Range& range);

// Widen the range by first checking if the induction variable is monotonically increasing.
// Requires "pRange" to be partially computed.
Expand Down
38 changes: 4 additions & 34 deletions src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,6 @@ internal ref TValue FindValue(TKey key)
i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional.
do
{
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test in if to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
Expand Down Expand Up @@ -450,7 +449,6 @@ internal ref TValue FindValue(TKey key)
i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional.
do
{
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test in if to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
Expand Down Expand Up @@ -535,15 +533,8 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)
comparer == null)
{
// ValueType: Devirtualize with EqualityComparer<TKey>.Default intrinsic
while (true)
while ((uint)i < (uint)entries.Length)
{
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test uint in if rather than loop condition to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
break;
}

if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entries[i].key, key))
{
if (behavior == InsertionBehavior.OverwriteExisting)
Expand Down Expand Up @@ -574,15 +565,8 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)
else
{
Debug.Assert(comparer is not null);
while (true)
while ((uint)i < (uint)entries.Length)
{
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test uint in if rather than loop condition to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
break;
}

if (entries[i].hashCode == hashCode && comparer.Equals(entries[i].key, key))
{
if (behavior == InsertionBehavior.OverwriteExisting)
Expand Down Expand Up @@ -690,15 +674,8 @@ internal static class CollectionsMarshalHelper
comparer == null)
{
// ValueType: Devirtualize with EqualityComparer<TKey>.Default intrinsic
while (true)
while ((uint)i < (uint)entries.Length)
{
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test uint in if rather than loop condition to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
break;
}

if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entries[i].key, key))
{
exists = true;
Expand All @@ -720,15 +697,8 @@ internal static class CollectionsMarshalHelper
else
{
Debug.Assert(comparer is not null);
while (true)
while ((uint)i < (uint)entries.Length)
{
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
// Test uint in if rather than loop condition to drop range check for following array access
if ((uint)i >= (uint)entries.Length)
{
break;
}

if (entries[i].hashCode == hashCode && comparer.Equals(entries[i].key, key))
{
exists = true;
Expand Down

0 comments on commit cd0cd23

Please sign in to comment.