From da07e04e9538bf80d7b5770c6bfc5885d42b938e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 8 Apr 2024 17:58:02 +0200 Subject: [PATCH 1/3] JIT: Add support for bounds check no throw assertions in range check Fix #9422 --- src/coreclr/jit/assertionprop.cpp | 4 +- src/coreclr/jit/rangecheck.cpp | 19 +++++++++- .../System/Collections/Generic/Dictionary.cs | 38 ++----------------- 3 files changed, 24 insertions(+), 37 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index ebcb101663a30..7cccbca3eaa5d 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -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("]"); } diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 2328dba7d6368..9403045d21546 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -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 { @@ -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; } diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs index 6007efe778ce2..9b18d4a125ed2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs @@ -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) { @@ -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) { @@ -535,15 +533,8 @@ private bool TryInsert(TKey key, TValue value, InsertionBehavior behavior) comparer == null) { // ValueType: Devirtualize with EqualityComparer.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.Default.Equals(entries[i].key, key)) { if (behavior == InsertionBehavior.OverwriteExisting) @@ -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) @@ -690,15 +674,8 @@ internal static class CollectionsMarshalHelper comparer == null) { // ValueType: Devirtualize with EqualityComparer.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.Default.Equals(entries[i].key, key)) { exists = true; @@ -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; From 7dd280057e87150823b903d59972b50061e0bc02 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 9 Apr 2024 16:18:37 +0200 Subject: [PATCH 2/3] Utilize assertions in `DoesOverflow` --- src/coreclr/jit/rangecheck.cpp | 60 +++++++++++++++++++++++++--------- src/coreclr/jit/rangecheck.h | 34 ++++++++++++------- 2 files changed, 66 insertions(+), 28 deletions(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 9403045d21546..717c4c0bfb4f3 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -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; @@ -1252,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; } @@ -1296,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) @@ -1308,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, 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()) { @@ -1320,7 +1335,7 @@ bool RangeCheck::DoesPhiOverflow(BasicBlock* block, GenTree* expr) { continue; } - if (DoesOverflow(block, arg)) + if (DoesOverflow(block, arg, range)) { return true; } @@ -1328,17 +1343,30 @@ bool RangeCheck::DoesPhiOverflow(BasicBlock* block, GenTree* expr) 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); @@ -1360,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 @@ -1381,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); diff --git a/src/coreclr/jit/rangecheck.h b/src/coreclr/jit/rangecheck.h index 9d7b064387174..de44b88d3ae52 100644 --- a/src/coreclr/jit/rangecheck.h +++ b/src/coreclr/jit/rangecheck.h @@ -199,7 +199,7 @@ struct Limit return false; } - bool Equals(Limit& l) + bool Equals(const Limit& l) const { switch (type) { @@ -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; @@ -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); @@ -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. From e5cd7cb3dc22d13b414d789ff515a499a0f4719a Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 9 Apr 2024 16:53:05 +0200 Subject: [PATCH 3/3] Fix release build --- src/coreclr/jit/rangecheck.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 717c4c0bfb4f3..e1046de8b9986 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -1312,7 +1312,7 @@ bool RangeCheck::DoesVarDefOverflow(BasicBlock* block, GenTreeLclVarCommon* lcl, // 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, 0); + 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