Skip to content

Commit 69110bf

Browse files
authored
JIT: Add support for bounds check no throw assertions in range check and make overflow check more precise (#100777)
Fix #9422 Fix #83349
1 parent f930b15 commit 69110bf

File tree

4 files changed

+90
-65
lines changed

4 files changed

+90
-65
lines changed

src/coreclr/jit/assertionprop.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -769,9 +769,9 @@ void Compiler::optPrintAssertion(AssertionDsc* curAssertion, AssertionIndex asse
769769
}
770770
else if (curAssertion->op1.kind == O1K_ARR_BND)
771771
{
772-
printf("[idx:");
772+
printf("[idx: " FMT_VN, curAssertion->op1.bnd.vnIdx);
773773
vnStore->vnDump(this, curAssertion->op1.bnd.vnIdx);
774-
printf(";len:");
774+
printf("; len: " FMT_VN, curAssertion->op1.bnd.vnLen);
775775
vnStore->vnDump(this, curAssertion->op1.bnd.vnLen);
776776
printf("]");
777777
}

src/coreclr/jit/rangecheck.cpp

Lines changed: 62 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -353,7 +353,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
353353
return;
354354
}
355355

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

774774
isConstantAssertion = true;
775775
}
776+
// Current assertion asserts a bounds check does not throw
777+
else if (curAssertion->IsBoundsCheckNoThrow())
778+
{
779+
ValueNum indexVN = curAssertion->op1.bnd.vnIdx;
780+
ValueNum lenVN = curAssertion->op1.bnd.vnLen;
781+
if (normalLclVN == indexVN)
782+
{
783+
isUnsigned = true;
784+
cmpOper = GT_LT;
785+
limit = Limit(Limit::keBinOpArray, lenVN, 0);
786+
}
787+
else
788+
{
789+
continue;
790+
}
791+
}
776792
// Current assertion is not supported, ignore it
777793
else
778794
{
@@ -782,7 +798,8 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse
782798
assert(limit.IsBinOpArray() || limit.IsConstant());
783799

784800
// Make sure the assertion is of the form != 0 or == 0 if it isn't a constant assertion.
785-
if (!isConstantAssertion && (curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)))
801+
if (!isConstantAssertion && (curAssertion->assertionKind != Compiler::OAK_NO_THROW) &&
802+
(curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)))
786803
{
787804
continue;
788805
}
@@ -1235,17 +1252,17 @@ bool RangeCheck::MultiplyOverflows(Limit& limit1, Limit& limit2)
12351252
}
12361253

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

1243-
if (!m_pSearchPath->Lookup(op1) && DoesOverflow(block, op1))
1260+
if (!m_pSearchPath->Lookup(op1) && DoesOverflow(block, op1, range))
12441261
{
12451262
return true;
12461263
}
12471264

1248-
if (!m_pSearchPath->Lookup(op2) && DoesOverflow(block, op2))
1265+
if (!m_pSearchPath->Lookup(op2) && DoesOverflow(block, op2, range))
12491266
{
12501267
return true;
12511268
}
@@ -1279,7 +1296,7 @@ bool RangeCheck::DoesBinOpOverflow(BasicBlock* block, GenTreeOp* binop)
12791296
}
12801297

12811298
// Check if the var definition the rhs involves arithmetic that overflows.
1282-
bool RangeCheck::DoesVarDefOverflow(GenTreeLclVarCommon* lcl)
1299+
bool RangeCheck::DoesVarDefOverflow(BasicBlock* block, GenTreeLclVarCommon* lcl, const Range& range)
12831300
{
12841301
LclSsaVarDsc* ssaDef = GetSsaDefStore(lcl);
12851302
if (ssaDef == nullptr)
@@ -1291,10 +1308,25 @@ bool RangeCheck::DoesVarDefOverflow(GenTreeLclVarCommon* lcl)
12911308
}
12921309
return true;
12931310
}
1294-
return DoesOverflow(ssaDef->GetBlock(), ssaDef->GetDefNode()->Data());
1311+
1312+
// We can use intermediate assertions about the local to prove that any
1313+
// overflow on this path does not matter for the range computed.
1314+
Range assertionRange = Range(Limit(Limit::keUnknown));
1315+
MergeAssertion(block, lcl, &assertionRange DEBUGARG(0));
1316+
1317+
// But only if the range from the assertion is more strict than the global
1318+
// range computed; otherwise we might still have used the def's value to
1319+
// tighten the range of the global range.
1320+
Range merged = RangeOps::Merge(range, assertionRange, false);
1321+
if (merged.LowerLimit().Equals(range.LowerLimit()) && merged.UpperLimit().Equals(range.UpperLimit()))
1322+
{
1323+
return false;
1324+
}
1325+
1326+
return DoesOverflow(ssaDef->GetBlock(), ssaDef->GetDefNode()->Data(), range);
12951327
}
12961328

1297-
bool RangeCheck::DoesPhiOverflow(BasicBlock* block, GenTree* expr)
1329+
bool RangeCheck::DoesPhiOverflow(BasicBlock* block, GenTree* expr, const Range& range)
12981330
{
12991331
for (GenTreePhi::Use& use : expr->AsPhi()->Uses())
13001332
{
@@ -1303,25 +1335,38 @@ bool RangeCheck::DoesPhiOverflow(BasicBlock* block, GenTree* expr)
13031335
{
13041336
continue;
13051337
}
1306-
if (DoesOverflow(block, arg))
1338+
if (DoesOverflow(block, arg, range))
13071339
{
13081340
return true;
13091341
}
13101342
}
13111343
return false;
13121344
}
13131345

1314-
bool RangeCheck::DoesOverflow(BasicBlock* block, GenTree* expr)
1346+
//------------------------------------------------------------------------
1347+
// DoesOverflow: Check if the computation of "expr" may have overflowed.
1348+
//
1349+
// Arguments:
1350+
// block - the block that contains `expr`
1351+
// expr - expression to check overflow of
1352+
// range - range that we believe "expr" to be in without accounting for
1353+
// overflow; used to ignore potential overflow on paths where
1354+
// we can prove the value is in this range regardless.
1355+
//
1356+
// Return value:
1357+
// True if the computation may have involved an impactful overflow.
1358+
//
1359+
bool RangeCheck::DoesOverflow(BasicBlock* block, GenTree* expr, const Range& range)
13151360
{
13161361
bool overflows = false;
13171362
if (!GetOverflowMap()->Lookup(expr, &overflows))
13181363
{
1319-
overflows = ComputeDoesOverflow(block, expr);
1364+
overflows = ComputeDoesOverflow(block, expr, range);
13201365
}
13211366
return overflows;
13221367
}
13231368

1324-
bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr)
1369+
bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr, const Range& range)
13251370
{
13261371
JITDUMP("Does overflow [%06d]?\n", Compiler::dspTreeID(expr));
13271372
m_pSearchPath->Set(expr, block, SearchPath::Overwrite);
@@ -1343,17 +1388,17 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr)
13431388
}
13441389
else if (expr->OperIs(GT_COMMA))
13451390
{
1346-
overflows = ComputeDoesOverflow(block, expr->gtEffectiveVal());
1391+
overflows = ComputeDoesOverflow(block, expr->gtEffectiveVal(), range);
13471392
}
13481393
// Check if the var def has rhs involving arithmetic that overflows.
13491394
else if (expr->IsLocal())
13501395
{
1351-
overflows = DoesVarDefOverflow(expr->AsLclVarCommon());
1396+
overflows = DoesVarDefOverflow(block, expr->AsLclVarCommon(), range);
13521397
}
13531398
// Check if add overflows.
13541399
else if (expr->OperIs(GT_ADD, GT_MUL))
13551400
{
1356-
overflows = DoesBinOpOverflow(block, expr->AsOp());
1401+
overflows = DoesBinOpOverflow(block, expr->AsOp(), range);
13571402
}
13581403
// These operators don't overflow.
13591404
// Actually, GT_LSH can overflow so it depends on the analysis done in ComputeRangeForBinOp
@@ -1364,11 +1409,11 @@ bool RangeCheck::ComputeDoesOverflow(BasicBlock* block, GenTree* expr)
13641409
// Walk through phi arguments to check if phi arguments involve arithmetic that overflows.
13651410
else if (expr->OperIs(GT_PHI))
13661411
{
1367-
overflows = DoesPhiOverflow(block, expr);
1412+
overflows = DoesPhiOverflow(block, expr, range);
13681413
}
13691414
else if (expr->OperIs(GT_CAST))
13701415
{
1371-
overflows = ComputeDoesOverflow(block, expr->gtGetOp1());
1416+
overflows = ComputeDoesOverflow(block, expr->gtGetOp1(), range);
13721417
}
13731418
GetOverflowMap()->Set(expr, overflows, OverflowMap::Overwrite);
13741419
m_pSearchPath->Remove(expr);

src/coreclr/jit/rangecheck.h

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ struct Limit
199199
return false;
200200
}
201201

202-
bool Equals(Limit& l)
202+
bool Equals(const Limit& l) const
203203
{
204204
switch (type)
205205
{
@@ -262,11 +262,21 @@ struct Range
262262
{
263263
}
264264

265+
const Limit& UpperLimit() const
266+
{
267+
return uLimit;
268+
}
269+
265270
Limit& UpperLimit()
266271
{
267272
return uLimit;
268273
}
269274

275+
const Limit& LowerLimit() const
276+
{
277+
return lLimit;
278+
}
279+
270280
Limit& LowerLimit()
271281
{
272282
return lLimit;
@@ -440,12 +450,12 @@ struct RangeOps
440450

441451
// Given two ranges "r1" and "r2", do a Phi merge. If "monIncreasing" is true,
442452
// then ignore the dependent variables for the lower bound but not for the upper bound.
443-
static Range Merge(Range& r1, Range& r2, bool monIncreasing)
453+
static Range Merge(const Range& r1, const Range& r2, bool monIncreasing)
444454
{
445-
Limit& r1lo = r1.LowerLimit();
446-
Limit& r1hi = r1.UpperLimit();
447-
Limit& r2lo = r2.LowerLimit();
448-
Limit& r2hi = r2.UpperLimit();
455+
const Limit& r1lo = r1.LowerLimit();
456+
const Limit& r1hi = r1.UpperLimit();
457+
const Limit& r2lo = r2.LowerLimit();
458+
const Limit& r2hi = r2.UpperLimit();
449459

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

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

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

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

701-
bool ComputeDoesOverflow(BasicBlock* block, GenTree* expr);
711+
bool ComputeDoesOverflow(BasicBlock* block, GenTree* expr, const Range& range);
702712

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

706716
// Widen the range by first checking if the induction variable is monotonically increasing.
707717
// Requires "pRange" to be partially computed.

src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs

Lines changed: 4 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,6 @@ internal ref TValue FindValue(TKey key)
418418
i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional.
419419
do
420420
{
421-
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
422421
// Test in if to drop range check for following array access
423422
if ((uint)i >= (uint)entries.Length)
424423
{
@@ -450,7 +449,6 @@ internal ref TValue FindValue(TKey key)
450449
i--; // Value in _buckets is 1-based; subtract 1 from i. We do it here so it fuses with the following conditional.
451450
do
452451
{
453-
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
454452
// Test in if to drop range check for following array access
455453
if ((uint)i >= (uint)entries.Length)
456454
{
@@ -535,15 +533,8 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)
535533
comparer == null)
536534
{
537535
// ValueType: Devirtualize with EqualityComparer<TKey>.Default intrinsic
538-
while (true)
536+
while ((uint)i < (uint)entries.Length)
539537
{
540-
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
541-
// Test uint in if rather than loop condition to drop range check for following array access
542-
if ((uint)i >= (uint)entries.Length)
543-
{
544-
break;
545-
}
546-
547538
if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entries[i].key, key))
548539
{
549540
if (behavior == InsertionBehavior.OverwriteExisting)
@@ -574,15 +565,8 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior)
574565
else
575566
{
576567
Debug.Assert(comparer is not null);
577-
while (true)
568+
while ((uint)i < (uint)entries.Length)
578569
{
579-
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
580-
// Test uint in if rather than loop condition to drop range check for following array access
581-
if ((uint)i >= (uint)entries.Length)
582-
{
583-
break;
584-
}
585-
586570
if (entries[i].hashCode == hashCode && comparer.Equals(entries[i].key, key))
587571
{
588572
if (behavior == InsertionBehavior.OverwriteExisting)
@@ -690,15 +674,8 @@ internal static class CollectionsMarshalHelper
690674
comparer == null)
691675
{
692676
// ValueType: Devirtualize with EqualityComparer<TKey>.Default intrinsic
693-
while (true)
677+
while ((uint)i < (uint)entries.Length)
694678
{
695-
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
696-
// Test uint in if rather than loop condition to drop range check for following array access
697-
if ((uint)i >= (uint)entries.Length)
698-
{
699-
break;
700-
}
701-
702679
if (entries[i].hashCode == hashCode && EqualityComparer<TKey>.Default.Equals(entries[i].key, key))
703680
{
704681
exists = true;
@@ -720,15 +697,8 @@ internal static class CollectionsMarshalHelper
720697
else
721698
{
722699
Debug.Assert(comparer is not null);
723-
while (true)
700+
while ((uint)i < (uint)entries.Length)
724701
{
725-
// Should be a while loop https://github.com/dotnet/runtime/issues/9422
726-
// Test uint in if rather than loop condition to drop range check for following array access
727-
if ((uint)i >= (uint)entries.Length)
728-
{
729-
break;
730-
}
731-
732702
if (entries[i].hashCode == hashCode && comparer.Equals(entries[i].key, key))
733703
{
734704
exists = true;

0 commit comments

Comments
 (0)