From 9c4c520cfe6d5a017af8d83c2f6f72bff2e7c327 Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Sun, 16 Aug 2020 17:11:25 -0400 Subject: [PATCH 01/14] Introduce a concept of minimum array length into range check --- src/coreclr/src/jit/assertionprop.cpp | 23 +- src/coreclr/src/jit/rangecheck.cpp | 102 ++++--- src/coreclr/src/jit/rangecheck.h | 27 +- .../src/System/String.cs | 6 +- .../AssertionPropagation/ArrBoundMinLength.cs | 288 ++++++++++++++++++ .../ArrBoundMinLength.csproj | 12 + 6 files changed, 410 insertions(+), 48 deletions(-) create mode 100644 src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs create mode 100644 src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.csproj diff --git a/src/coreclr/src/jit/assertionprop.cpp b/src/coreclr/src/jit/assertionprop.cpp index 7bac5584bf92c9..4737418e53822b 100644 --- a/src/coreclr/src/jit/assertionprop.cpp +++ b/src/coreclr/src/jit/assertionprop.cpp @@ -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: @@ -1959,12 +1958,34 @@ AssertionInfo Compiler::optAssertionGenJtrue(GenTree* tree) { std::swap(op1, op2); } + + ValueNum op1VN = vnStore->VNConservativeNormalValue(op1->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) && op2->OperIs(GT_CNS_INT)) + { + AssertionDsc dsc; + dsc.assertionKind = OAK_EQUAL; + dsc.op1.vn = vnStore->VNConservativeNormalValue(relop->gtVNPair); + dsc.op1.kind = O1K_ARR_BND; + dsc.op1.bnd.vnIdx = vnStore->VNForIntCon(op2->AsIntCon()->IntegralValue() - 1); + 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; + + AssertionIndex index = optAddAssertion(&dsc); + if (relop->OperIs(GT_NE)) + { + return AssertionInfo::ForNextEdge(index); + } + 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)) && diff --git a/src/coreclr/src/jit/rangecheck.cpp b/src/coreclr/src/jit/rangecheck.cpp index 66ebafb1c747aa..26cab8457d95e6 100644 --- a/src/coreclr/src/jit/rangecheck.cpp +++ b/src/coreclr/src/jit/rangecheck.cpp @@ -5,6 +5,7 @@ #include "jitpch.h" #include "rangecheck.h" +#include "compiler.h" // Max stack depth (path length) in walking the UD chain. static const int MAX_SEARCH_DEPTH = 100; @@ -60,7 +61,7 @@ int RangeCheck::GetArrLength(ValueNum vn) } // Check if the computed range is within bounds. -bool RangeCheck::BetweenBounds(Range& range, int lower, GenTree* upper) +bool RangeCheck::BetweenBounds(Range& range, int lower, ValueNum uLimitVN, int arrSize DEBUGARG(GenTree* upper)) { #ifdef DEBUG if (m_pCompiler->verbose) @@ -73,10 +74,8 @@ bool RangeCheck::BetweenBounds(Range& range, int lower, GenTree* upper) ValueNumStore* vnStore = m_pCompiler->vnStore; - // Get the VN for the upper limit. - ValueNum uLimitVN = vnStore->VNConservativeNormalValue(upper->gtVNPair); - #ifdef DEBUG + assert(vnStore->VNConservativeNormalValue(upper->gtVNPair)); JITDUMP(FMT_VN " upper bound is: ", uLimitVN); if (m_pCompiler->verbose) { @@ -85,26 +84,7 @@ bool RangeCheck::BetweenBounds(Range& range, int lower, GenTree* upper) JITDUMP("\n"); #endif - int arrSize = 0; - - if (vnStore->IsVNConstant(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 ((arrSize <= 0) && !vnStore->IsVNCheckedBound(uLimitVN)) { // If the upper limit is not length, then bail. return false; @@ -231,6 +211,16 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree* #endif // FEATURE_SIMD { arrSize = GetArrLength(arrLenVn); + if (arrSize <= 0) + { + // see if there are any assertions about the array size we can use + 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); @@ -286,7 +276,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, 0, arrLenVn, arrSize DEBUGARG(bndsChk->gtArrLen))) { JITDUMP("[RangeCheck::OptimizeRangeCheck] Between bounds\n"); m_pCompiler->optRemoveRangeCheck(treeParent, stmt); @@ -532,7 +522,6 @@ void RangeCheck::SetDef(UINT64 hash, Location* loc) } #endif -// Merge assertions on the edge flowing into the block about a variable. void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP assertions, Range* pRange) { if (BitVecOps::IsEmpty(m_pCompiler->apTraits, assertions)) @@ -544,6 +533,20 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP { 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); +} + +// Merge assertions on the edge flowing into the block about a variable. +void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP assertions, Range* pRange) +{ // Walk through the "assertions" to check if the apply. BitVecOps::Iter iter(m_pCompiler->apTraits, assertions); unsigned index = 0; @@ -556,14 +559,6 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP 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); - // Current assertion is of the form (i < len - cns) != 0 if (curAssertion->IsCheckedBoundArithBound()) { @@ -602,13 +597,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 (info.vnBound == 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()) @@ -627,6 +629,11 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP limit = Limit(Limit::keConstant, info.constVal); cmpOper = (genTreeOps)info.cmpOper; } + else if (IsConstantAssertion(curAssertion, normalLclVN)) + { + limit = Limit(Limit::keConstant, m_pCompiler->vnStore->ConstantValue(curAssertion->op2.vn)); + cmpOper = GT_EQ; + } // Current assertion is not supported, ignore it else { @@ -636,7 +643,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)) + if ((curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)) && + (cmpOper != GT_EQ)) { continue; } @@ -647,6 +655,13 @@ 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 = Limit(Limit::keConstant, m_pCompiler->vnStore->ConstantValue(limit.vn) + limit.cns); + } + ValueNum arrLenVN = m_pCompiler->vnStore->VNConservativeNormalValue(m_pCurBndsChk->gtArrLen->gtVNPair); if (m_pCompiler->vnStore->IsVNConstant(arrLenVN)) @@ -663,6 +678,7 @@ 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. @@ -673,12 +689,12 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP // If we have an 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) && (cmpOper != GT_EQ)) { cmpOper = GenTree::ReverseRelop(cmpOper); } - // Bounds are inclusive, so add -1 for upper bound when "<". But make sure we won't overflow. + // 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; @@ -744,6 +760,10 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP pRange->lLimit = limit; break; + case GT_EQ: + pRange->uLimit = limit; + pRange->lLimit = limit; + default: // All other 'cmpOper' kinds leave lLimit/uLimit unchanged break; diff --git a/src/coreclr/src/jit/rangecheck.h b/src/coreclr/src/jit/rangecheck.h index 754f64cbc2ea6b..ac013a4558c738 100644 --- a/src/coreclr/src/jit/rangecheck.h +++ b/src/coreclr/src/jit/rangecheck.h @@ -165,6 +165,7 @@ struct Limit } return false; } + #ifdef DEBUG const char* ToString(CompAllocator alloc) { @@ -376,6 +377,21 @@ struct RangeOps result.uLimit = r2hi; } } + if (r1lo.IsBinOpArray() && r2lo.IsBinOpArray() && r1lo.vn == r2lo.vn) + { + result.lLimit = r2lo.GetConstant() < r1lo.GetConstant() ? r2lo : r1lo; + } + + if (r1lo.IsConstant() && r1lo.GetConstant() >= 0 && r2lo.IsBinOpArray() && + r2lo.GetConstant() >= r1lo.GetConstant()) + { + result.lLimit = r1lo; + } + else if (r2lo.IsConstant() && r2lo.GetConstant() >= 0 && r1lo.IsBinOpArray() && + r1lo.GetConstant() >= r2lo.GetConstant()) + { + result.lLimit = r2lo; + } return result; } }; @@ -438,7 +454,7 @@ class RangeCheck // 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 BetweenBounds(Range& range, int lower, GenTree* upper); + bool BetweenBounds(Range& range, int lower, ValueNum uLimitVN, int arrSize DEBUGARG(GenTree* upper)); // Entry point to optimize range checks in the block. Assumes value numbering // and assertion prop phases are completed. @@ -474,6 +490,8 @@ class RangeCheck // refine the "pRange" value. void MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP assertions, Range* pRange); + void MergeEdgeAssertions(ValueNum num, ASSERT_VALARG_TP, Range* pRange); + // The maximum possible value of the given "limit." If such a value could not be determined // return "false." For example: ARRLEN_MAX for array length. bool GetLimitMax(Limit& limit, int* pMax); @@ -519,6 +537,13 @@ class RangeCheck GenTreeBoundsChk* m_pCurBndsChk; + // Is the given assertion a constant assertion with the given vn + bool IsConstantAssertion(Compiler::AssertionDsc* dsc, ValueNum vn) + { + return (dsc->assertionKind == Compiler::OAK_EQUAL) && (dsc->op1.vn == vn) + && (m_pCompiler->vnStore->IsVNInt32Constant(dsc->op2.vn)); + } + // Get the cached overflow values. OverflowMap* GetOverflowMap(); OverflowMap* m_pOverflowMap; diff --git a/src/libraries/System.Private.CoreLib/src/System/String.cs b/src/libraries/System.Private.CoreLib/src/System/String.cs index 54333a3c215d9a..510c1fe97f1263 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.cs @@ -487,13 +487,9 @@ public char[] ToCharArray(int startIndex, int length) [NonVersionable] public static bool IsNullOrEmpty([NotNullWhen(false)] string? value) { - // Using 0u >= (uint)value.Length rather than - // value.Length == 0 as it will elide the bounds check to - // the first char: value[0] if that is performed following the test - // for the same test cost. // Ternary operator returning true/false prevents redundant asm generation: // https://github.com/dotnet/runtime/issues/4207 - return (value == null || 0u >= (uint)value.Length) ? true : false; + return (value == null || 0 == value.Length) ? true : false; } public static bool IsNullOrWhiteSpace([NotNullWhen(false)] string? value) diff --git a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs new file mode 100644 index 00000000000000..758a935c06a639 --- /dev/null +++ b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs @@ -0,0 +1,288 @@ + +using System; + +class Program +{ + private static int returnCode = 100; + + private readonly static int[] arr = new int[6]; + + public static int Main(string[] args) + { + RunTestThrows(Tests.GreaterOutOfBound); + RunTestThrows(Tests.GreaterEqualOutOfBound); + RunTestThrows(Tests.LessOutOfBound); + RunTestThrows(Tests.LessEqualsOutOfBound); + RunTestThrows(Tests.EqualsOutOfBound); + RunTestThrows(Tests.EqualsReversedOutOfBound); + RunTestThrows(Tests.NotEqualsOutOfBound); + RunTestThrows(TestsEarlyReturn.GreaterOutOfBound); + RunTestThrows(TestsEarlyReturn.GreaterEqualOutOfBound); + RunTestThrows(TestsEarlyReturn.LessOutOfBound); + RunTestThrows(TestsEarlyReturn.LessEqualsOutOfBound); + RunTestThrows(TestsEarlyReturn.NotEqualsOutOfBound); + RunTestThrows(TestsEarlyReturn.EqualsOutOfBound); + + RunTestNoThrow(Tests.GreaterInBound); + RunTestNoThrow(Tests.GreaterEqualInBound); + RunTestNoThrow(Tests.LessInBound); + RunTestNoThrow(Tests.EqualsInBound); + RunTestNoThrow(Tests.LessEqualsInBound); + RunTestNoThrow(Tests.EqualsInBound); + RunTestNoThrow(Tests.EqualsReversedInBound); + RunTestNoThrow(TestsEarlyReturn.GreaterInBound); + RunTestNoThrow(TestsEarlyReturn.GreaterEqualInBound); + RunTestNoThrow(TestsEarlyReturn.LessInBound); + RunTestNoThrow(TestsEarlyReturn.LessEqualsInBound); + RunTestNoThrow(TestsEarlyReturn.NotEqualsInBound); + + return returnCode; + } + + private static void RunTestThrows(Action action) + { + try + { + action(arr); + Console.WriteLine("failed " + action.Method.Name); + returnCode--; + } + catch (Exception) + { + + } + } + + private static void RunTestNoThrow(Action action) + { + try + { + action(arr); + } + catch (Exception) + { + Console.WriteLine("failed " + action.Method.Name); + returnCode--; + } + } +} + +public static class Tests +{ + public static void GreaterInBound(int[] arr) + { + if (arr.Length > 5) + { + arr[5] = 1; + } + } + + public static void GreaterOutOfBound(int[] arr) + { + if (arr.Length > 5) + { + arr[6] = 1; + } + } + + public static void GreaterEqualInBound(int[] arr) + { + if (arr.Length >= 6) + { + arr[5] = 1; + } + } + + public static void GreaterEqualOutOfBound(int[] arr) + { + if (arr.Length >= 5) + { + arr[6] = 1; + } + } + + public static void LessInBound(int[] arr) + { + if (5 < arr.Length) + { + arr[5] = 1; + } + } + + public static void LessOutOfBound(int[] arr) + { + if (5 < arr.Length) + { + arr[6] = 1; + } + } + + public static void LessEqualsInBound(int[] arr) + { + if (5 <= arr.Length) + { + arr[4] = 1; + } + } + + public static void LessEqualsOutOfBound(int[] arr) + { + if (5 <= arr.Length) + { + arr[6] = 1; + } + } + + public static void EqualsInBound(int[] arr) + { + if (arr.Length == 6) + { + arr[5] = 1; + } + } + + public static void EqualsReversedInBound(int[] arr) + { + if (6 == arr.Length) + { + arr[5] = 1; + } + } + + public static void EqualsOutOfBound(int[] arr) + { + if (arr.Length == 6) + { + arr[6] = 1; + } + } + + public static void EqualsReversedOutOfBound(int[] arr) + { + if (6 == arr.Length) + { + arr[6] = 1; + } + } + + public static void NotEqualsOutOfBound(int[] arr) + { + if (arr.Length != 5) + { + arr[6] = 1; + } + } +} + +public static class TestsEarlyReturn +{ + public static void GreaterInBound(int[] arr) + { + if (arr.Length <= 5) + { + return; + } + + arr[5] = 1; + } + + public static void GreaterOutOfBound(int[] arr) + { + if (arr.Length <= 5) + { + return; + } + + arr[6] = 1; + } + + public static void GreaterEqualInBound(int[] arr) + { + if (arr.Length < 5) + { + return; + } + + arr[4] = 1; + } + + public static void GreaterEqualOutOfBound(int[] arr) + { + if (arr.Length < 5) + { + return; + } + + arr[6] = 1; + } + + public static void LessInBound(int[] arr) + { + if (5 >= arr.Length) + { + return; + } + + arr[5] = 1; + } + + public static void LessOutOfBound(int[] arr) + { + if (5 >= arr.Length) + { + return; + } + + arr[6] = 1; + } + + public static void LessEqualsInBound(int[] arr) + { + if (5 > arr.Length) + { + return; + } + + arr[4] = 1; + } + + public static void LessEqualsOutOfBound(int[] arr) + { + if (5 > arr.Length) + { + return; + } + + arr[6] = 1; + } + + public static void NotEqualsInBound(int[] arr) + { + if (arr.Length != 6) + { + return; + } + + arr[5] = 1; + } + + public static void NotEqualsOutOfBound(int[] arr) + { + if (arr.Length != 6) + { + return; + } + + arr[6] = 1; + } + + public static void EqualsOutOfBound(int[] arr) + { + if (arr.Length == 5) + { + return; + } + + arr[6] = 1; + } +} \ No newline at end of file diff --git a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.csproj b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.csproj new file mode 100644 index 00000000000000..5c20dcadd57cac --- /dev/null +++ b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.csproj @@ -0,0 +1,12 @@ + + + Exe + + + None + True + + + + + From bce02acfe6165db34a9f4f84107cbfe60b7ea62e Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Sat, 22 Aug 2020 12:28:30 -0400 Subject: [PATCH 02/14] Some cleanup --- src/coreclr/src/jit/assertionprop.cpp | 5 ++- src/coreclr/src/jit/compiler.h | 5 +++ src/coreclr/src/jit/rangecheck.cpp | 30 +++++++++---- src/coreclr/src/jit/rangecheck.h | 15 ++----- .../AssertionPropagation/ArrBoundMinLength.cs | 42 +++++++++++++++++++ 5 files changed, 75 insertions(+), 22 deletions(-) diff --git a/src/coreclr/src/jit/assertionprop.cpp b/src/coreclr/src/jit/assertionprop.cpp index 4737418e53822b..8ac9b18474d5d5 100644 --- a/src/coreclr/src/jit/assertionprop.cpp +++ b/src/coreclr/src/jit/assertionprop.cpp @@ -1960,19 +1960,20 @@ AssertionInfo Compiler::optAssertionGenJtrue(GenTree* tree) } 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) && op2->OperIs(GT_CNS_INT)) + else if (vnStore->IsVNCheckedBound(op1VN) && vnStore->IsVNInt32Constant(op2VN)) { AssertionDsc dsc; dsc.assertionKind = OAK_EQUAL; dsc.op1.vn = vnStore->VNConservativeNormalValue(relop->gtVNPair); dsc.op1.kind = O1K_ARR_BND; - dsc.op1.bnd.vnIdx = vnStore->VNForIntCon(op2->AsIntCon()->IntegralValue() - 1); + dsc.op1.bnd.vnIdx = vnStore->VNForIntCon(vnStore->ConstantValue(op2VN) - 1); dsc.op1.bnd.vnLen = op1VN; dsc.op2.vn = vnStore->VNConservativeNormalValue(op2->gtVNPair); dsc.op2.kind = O2K_CONST_INT; diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 1c6f87851f52fe..82e4cab6d31224 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -6787,6 +6787,11 @@ class Compiler return ((assertionKind == OAK_EQUAL) && (op1.kind == O1K_LCLVAR) && (op2.kind == O2K_LCLVAR_COPY)); } + bool IsConstantInt32Assertion() + { + return (assertionKind == OAK_EQUAL) && (op2.kind == O2K_CONST_INT); + } + static bool SameKind(AssertionDsc* a1, AssertionDsc* a2) { return a1->assertionKind == a2->assertionKind && a1->op1.kind == a2->op1.kind && diff --git a/src/coreclr/src/jit/rangecheck.cpp b/src/coreclr/src/jit/rangecheck.cpp index 26cab8457d95e6..33d4c66e09e139 100644 --- a/src/coreclr/src/jit/rangecheck.cpp +++ b/src/coreclr/src/jit/rangecheck.cpp @@ -5,7 +5,6 @@ #include "jitpch.h" #include "rangecheck.h" -#include "compiler.h" // Max stack depth (path length) in walking the UD chain. static const int MAX_SEARCH_DEPTH = 100; @@ -61,7 +60,7 @@ int RangeCheck::GetArrLength(ValueNum vn) } // Check if the computed range is within bounds. -bool RangeCheck::BetweenBounds(Range& range, int lower, ValueNum uLimitVN, int arrSize DEBUGARG(GenTree* upper)) +bool RangeCheck::BetweenBounds(Range& range, int lower, GenTree* upper, int arrSize) { #ifdef DEBUG if (m_pCompiler->verbose) @@ -74,8 +73,10 @@ bool RangeCheck::BetweenBounds(Range& range, int lower, ValueNum uLimitVN, int a ValueNumStore* vnStore = m_pCompiler->vnStore; + // Get the VN for the upper limit. + ValueNum uLimitVN = vnStore->VNConservativeNormalValue(upper->gtVNPair); + #ifdef DEBUG - assert(vnStore->VNConservativeNormalValue(upper->gtVNPair)); JITDUMP(FMT_VN " upper bound is: ", uLimitVN); if (m_pCompiler->verbose) { @@ -214,11 +215,13 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree* if (arrSize <= 0) { // see if there are any assertions about the array size we can use + JITDUMP("Looking for 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("Min array size is %d", arrSize); } } } @@ -276,7 +279,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree* } // Is the range between the lower and upper bound values. - if (BetweenBounds(range, 0, arrLenVn, arrSize DEBUGARG(bndsChk->gtArrLen))) + if (BetweenBounds(range, 0, bndsChk->gtArrLen, arrSize)) { JITDUMP("[RangeCheck::OptimizeRangeCheck] Between bounds\n"); m_pCompiler->optRemoveRangeCheck(treeParent, stmt); @@ -629,9 +632,15 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse limit = Limit(Limit::keConstant, info.constVal); cmpOper = (genTreeOps)info.cmpOper; } - else if (IsConstantAssertion(curAssertion, normalLclVN)) + // Current assertion is of the form i == 100 + else if (curAssertion->IsConstantInt32Assertion()) { - limit = Limit(Limit::keConstant, m_pCompiler->vnStore->ConstantValue(curAssertion->op2.vn)); + if (curAssertion->op1.vn != normalLclVN) + { + continue; + } + + limit = Limit(Limit::keConstant, m_pCompiler->vnStore->ConstantValue(curAssertion->op2.vn)); cmpOper = GT_EQ; } // Current assertion is not supported, ignore it @@ -643,8 +652,7 @@ 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 ((curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)) && - (cmpOper != GT_EQ)) + if ((curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)) && (cmpOper != GT_EQ)) { continue; } @@ -659,7 +667,11 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse // see if we can simplify this to just a constant if (limit.IsBinOpArray() && m_pCompiler->vnStore->IsVNInt32Constant(limit.vn)) { - limit = Limit(Limit::keConstant, m_pCompiler->vnStore->ConstantValue(limit.vn) + limit.cns); + Limit tempLimit = Limit(Limit::keConstant, m_pCompiler->vnStore->ConstantValue(limit.vn)); + if (tempLimit.AddConstant(limit.cns)) + { + limit = tempLimit; + } } ValueNum arrLenVN = m_pCompiler->vnStore->VNConservativeNormalValue(m_pCurBndsChk->gtArrLen->gtVNPair); diff --git a/src/coreclr/src/jit/rangecheck.h b/src/coreclr/src/jit/rangecheck.h index ac013a4558c738..fb12f3e4b743d1 100644 --- a/src/coreclr/src/jit/rangecheck.h +++ b/src/coreclr/src/jit/rangecheck.h @@ -165,7 +165,6 @@ struct Limit } return false; } - #ifdef DEBUG const char* ToString(CompAllocator alloc) { @@ -388,7 +387,7 @@ struct RangeOps result.lLimit = r1lo; } else if (r2lo.IsConstant() && r2lo.GetConstant() >= 0 && r1lo.IsBinOpArray() && - r1lo.GetConstant() >= r2lo.GetConstant()) + r1lo.GetConstant() >= r2lo.GetConstant()) { result.lLimit = r2lo; } @@ -454,7 +453,7 @@ class RangeCheck // 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 BetweenBounds(Range& range, int lower, ValueNum uLimitVN, int arrSize DEBUGARG(GenTree* upper)); + bool BetweenBounds(Range& range, int lower, GenTree* upper, int arrSize); // Entry point to optimize range checks in the block. Assumes value numbering // and assertion prop phases are completed. @@ -490,7 +489,8 @@ class RangeCheck // refine the "pRange" value. void MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP assertions, Range* pRange); - void MergeEdgeAssertions(ValueNum num, ASSERT_VALARG_TP, Range* pRange); + // Inspect the assertions about the current ValueNum to refine pRange + void MergeEdgeAssertions(ValueNum num, ASSERT_VALARG_TP assertions, Range* pRange); // The maximum possible value of the given "limit." If such a value could not be determined // return "false." For example: ARRLEN_MAX for array length. @@ -537,13 +537,6 @@ class RangeCheck GenTreeBoundsChk* m_pCurBndsChk; - // Is the given assertion a constant assertion with the given vn - bool IsConstantAssertion(Compiler::AssertionDsc* dsc, ValueNum vn) - { - return (dsc->assertionKind == Compiler::OAK_EQUAL) && (dsc->op1.vn == vn) - && (m_pCompiler->vnStore->IsVNInt32Constant(dsc->op2.vn)); - } - // Get the cached overflow values. OverflowMap* GetOverflowMap(); OverflowMap* m_pOverflowMap; diff --git a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs index 758a935c06a639..220f03418cc5e4 100644 --- a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs +++ b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs @@ -1,3 +1,4 @@ +using System.Runtime.CompilerServices; using System; @@ -36,6 +37,9 @@ public static int Main(string[] args) RunTestNoThrow(TestsEarlyReturn.LessEqualsInBound); RunTestNoThrow(TestsEarlyReturn.NotEqualsInBound); + Tests.ModInBounds(arr, 11); + try { Tests.ModOutOfBounds(arr, 6); returnCode--; } catch {} + return returnCode; } @@ -135,6 +139,8 @@ public static void LessEqualsOutOfBound(int[] arr) public static void EqualsInBound(int[] arr) { + arr[0] = 1; + if (arr.Length == 6) { arr[5] = 1; @@ -143,6 +149,8 @@ public static void EqualsInBound(int[] arr) public static void EqualsReversedInBound(int[] arr) { + arr[0] = 1; + if (6 == arr.Length) { arr[5] = 1; @@ -151,6 +159,8 @@ public static void EqualsReversedInBound(int[] arr) public static void EqualsOutOfBound(int[] arr) { + arr[0] = 1; + if (arr.Length == 6) { arr[6] = 1; @@ -159,6 +169,8 @@ public static void EqualsOutOfBound(int[] arr) public static void EqualsReversedOutOfBound(int[] arr) { + arr[0] = 1; + if (6 == arr.Length) { arr[6] = 1; @@ -167,11 +179,35 @@ public static void EqualsReversedOutOfBound(int[] arr) public static void NotEqualsOutOfBound(int[] arr) { + arr[0] = 1; + if (arr.Length != 5) { arr[6] = 1; } } + + [MethodImplAttribute(MethodImplOptions.NoInlining)] + public static void ModInBounds(int[] arr, uint i) + { + arr[0] = 1; //needed to signal that arr isn't null + + if (arr.Length == 6) + { + arr[(int)(i % 6)] = 0; + } + } + + [MethodImplAttribute(MethodImplOptions.NoInlining)] + public static void ModOutOfBounds(int[] arr, uint i) + { + arr[0] = 1; + + if (arr.Length == 6) + { + arr[(int)(i % 7)] = 0; + } + } } public static class TestsEarlyReturn @@ -258,6 +294,8 @@ public static void LessEqualsOutOfBound(int[] arr) public static void NotEqualsInBound(int[] arr) { + arr[0] = 1; + if (arr.Length != 6) { return; @@ -268,6 +306,8 @@ public static void NotEqualsInBound(int[] arr) public static void NotEqualsOutOfBound(int[] arr) { + arr[0] = 1; + if (arr.Length != 6) { return; @@ -278,6 +318,8 @@ public static void NotEqualsOutOfBound(int[] arr) public static void EqualsOutOfBound(int[] arr) { + arr[0] = 1; + if (arr.Length == 5) { return; From 45caddbe69848d0572f4fd539f4b24ce0d8e0e02 Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Sat, 22 Aug 2020 14:36:39 -0400 Subject: [PATCH 03/14] fix potential underflow --- src/coreclr/src/jit/assertionprop.cpp | 36 +++++++++++++++------------ src/coreclr/src/jit/rangecheck.cpp | 2 +- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/src/coreclr/src/jit/assertionprop.cpp b/src/coreclr/src/jit/assertionprop.cpp index 8ac9b18474d5d5..8a42844a05348b 100644 --- a/src/coreclr/src/jit/assertionprop.cpp +++ b/src/coreclr/src/jit/assertionprop.cpp @@ -1969,23 +1969,27 @@ AssertionInfo Compiler::optAssertionGenJtrue(GenTree* tree) } else if (vnStore->IsVNCheckedBound(op1VN) && vnStore->IsVNInt32Constant(op2VN)) { - AssertionDsc dsc; - dsc.assertionKind = OAK_EQUAL; - dsc.op1.vn = vnStore->VNConservativeNormalValue(relop->gtVNPair); - dsc.op1.kind = O1K_ARR_BND; - dsc.op1.bnd.vnIdx = vnStore->VNForIntCon(vnStore->ConstantValue(op2VN) - 1); - 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; - - AssertionIndex index = optAddAssertion(&dsc); - if (relop->OperIs(GT_NE)) - { - return AssertionInfo::ForNextEdge(index); + int con = vnStore->ConstantValue(op2VN); + if (con > 0) + { + AssertionDsc dsc; + dsc.assertionKind = OAK_EQUAL; + dsc.op1.vn = vnStore->VNConservativeNormalValue(relop->gtVNPair); + dsc.op1.kind = O1K_ARR_BND; + dsc.op1.bnd.vnIdx = vnStore->VNForIntCon(con - 1); + 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; + + AssertionIndex index = optAddAssertion(&dsc); + if (relop->OperIs(GT_NE)) + { + return AssertionInfo::ForNextEdge(index); + } + return index; } - return index; } // Check op1 and op2 for an indirection of a GT_LCL_VAR and keep it in op1. diff --git a/src/coreclr/src/jit/rangecheck.cpp b/src/coreclr/src/jit/rangecheck.cpp index 33d4c66e09e139..8a6a154241955c 100644 --- a/src/coreclr/src/jit/rangecheck.cpp +++ b/src/coreclr/src/jit/rangecheck.cpp @@ -221,7 +221,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree* if (arrLength.lLimit.IsConstant()) { arrSize = arrLength.lLimit.GetConstant(); - JITDUMP("Min array size is %d", arrSize); + JITDUMP("Min array size is %d\n", arrSize); } } } From aadeabecf27831c8ae86b1e7e6529c43e999129b Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Thu, 27 Aug 2020 15:46:14 -0400 Subject: [PATCH 04/14] bug fix and cleanup --- src/coreclr/src/jit/rangecheck.cpp | 20 +++++++++++-------- .../AssertionPropagation/ArrBoundMinLength.cs | 4 +++- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/coreclr/src/jit/rangecheck.cpp b/src/coreclr/src/jit/rangecheck.cpp index 8a6a154241955c..4401ca426276c6 100644 --- a/src/coreclr/src/jit/rangecheck.cpp +++ b/src/coreclr/src/jit/rangecheck.cpp @@ -215,13 +215,12 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree* if (arrSize <= 0) { // see if there are any assertions about the array size we can use - JITDUMP("Looking for assertions for: " FMT_VN "\n", arrLenVn); + 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("Min array size is %d\n", arrSize); } } } @@ -527,11 +526,6 @@ void RangeCheck::SetDef(UINT64 hash, Location* loc) void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP assertions, Range* pRange) { - if (BitVecOps::IsEmpty(m_pCompiler->apTraits, assertions)) - { - return; - } - if (lcl->GetSsaNum() == SsaConfig::RESERVED_SSA_NUM) { return; @@ -550,6 +544,16 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP // Merge assertions on the edge flowing into the block about a variable. void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP assertions, Range* pRange) { + if (BitVecOps::IsEmpty(m_pCompiler->apTraits, assertions)) + { + return; + } + + if (normalLclVN == ValueNumStore::NoVN) + { + return; + } + // Walk through the "assertions" to check if the apply. BitVecOps::Iter iter(m_pCompiler->apTraits, assertions); unsigned index = 0; @@ -605,7 +609,7 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse cmpOper = (genTreeOps)info.cmpOper; limit = Limit(Limit::keBinOpArray, info.vnBound, 0); } - else if (info.vnBound == info.vnBound) + else if (normalLclVN == info.vnBound) { cmpOper = GenTree::SwapRelop((genTreeOps)info.cmpOper); limit = Limit(Limit::keBinOpArray, info.cmpOp, 0); diff --git a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs index 220f03418cc5e4..bc4016282c495b 100644 --- a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs +++ b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs @@ -1,6 +1,8 @@ -using System.Runtime.CompilerServices; +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Runtime.CompilerServices; class Program { From 6f77bf8c8acce1f5382bb704875384c6f8e2f984 Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Thu, 27 Aug 2020 23:14:00 -0400 Subject: [PATCH 05/14] Revert string changes --- src/libraries/System.Private.CoreLib/src/System/String.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.cs b/src/libraries/System.Private.CoreLib/src/System/String.cs index 510c1fe97f1263..49af25beb49c3b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.cs @@ -487,9 +487,13 @@ public char[] ToCharArray(int startIndex, int length) [NonVersionable] public static bool IsNullOrEmpty([NotNullWhen(false)] string? value) { + // Using 0u >= (uint)value.Length rather than + // value.Length == 0 as it will elide the bounds check to + // the first char: value[0] if that is performed following the test + // for the same test cost. // Ternary operator returning true/false prevents redundant asm generation: // https://github.com/dotnet/runtime/issues/4207 - return (value == null || 0 == value.Length) ? true : false; + return (value == null || 0u >= (uint)value.Length) ? true : false; } public static bool IsNullOrWhiteSpace([NotNullWhen(false)] string? value) @@ -607,7 +611,6 @@ public StringRuneEnumerator EnumerateRunes() internal static unsafe int wcslen(char* ptr) { // IndexOf processes memory in aligned chunks, and thus it won't crash even if it accesses memory beyond the null terminator. - // This IndexOf behavior is an implementation detail of the runtime and callers outside System.Private.CoreLib must not depend on it. int length = SpanHelpers.IndexOf(ref *ptr, '\0', int.MaxValue); if (length < 0) { @@ -621,7 +624,6 @@ internal static unsafe int wcslen(char* ptr) internal static unsafe int strlen(byte* ptr) { // IndexOf processes memory in aligned chunks, and thus it won't crash even if it accesses memory beyond the null terminator. - // This IndexOf behavior is an implementation detail of the runtime and callers outside System.Private.CoreLib must not depend on it. int length = SpanHelpers.IndexOf(ref *ptr, (byte)'\0', int.MaxValue); if (length < 0) { From 0ec7beac49059f08d1c64da0bfc2e820f5fd0085 Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Thu, 27 Aug 2020 23:12:27 -0400 Subject: [PATCH 06/14] Allow elimination of arr[0] with len != 0 test --- src/coreclr/src/jit/assertionprop.cpp | 19 ++++-- src/coreclr/src/jit/compiler.h | 2 +- src/coreclr/src/jit/rangecheck.cpp | 22 +++++-- .../AssertionPropagation/ArrBoundMinLength.cs | 58 +++++++++++++++++++ 4 files changed, 91 insertions(+), 10 deletions(-) diff --git a/src/coreclr/src/jit/assertionprop.cpp b/src/coreclr/src/jit/assertionprop.cpp index 8a42844a05348b..b554d44416addb 100644 --- a/src/coreclr/src/jit/assertionprop.cpp +++ b/src/coreclr/src/jit/assertionprop.cpp @@ -1970,13 +1970,22 @@ AssertionInfo Compiler::optAssertionGenJtrue(GenTree* tree) else if (vnStore->IsVNCheckedBound(op1VN) && vnStore->IsVNInt32Constant(op2VN)) { int con = vnStore->ConstantValue(op2VN); - if (con > 0) + if (con >= 0) { AssertionDsc dsc; - dsc.assertionKind = OAK_EQUAL; - dsc.op1.vn = vnStore->VNConservativeNormalValue(relop->gtVNPair); + 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.vnIdx = vnStore->VNForIntCon(con - 1); dsc.op1.bnd.vnLen = op1VN; dsc.op2.vn = vnStore->VNConservativeNormalValue(op2->gtVNPair); dsc.op2.kind = O2K_CONST_INT; @@ -1984,7 +1993,7 @@ AssertionInfo Compiler::optAssertionGenJtrue(GenTree* tree) dsc.op2.u1.iconVal = 0; AssertionIndex index = optAddAssertion(&dsc); - if (relop->OperIs(GT_NE)) + if (relop->OperIs(GT_NE) != (con == 0)) { return AssertionInfo::ForNextEdge(index); } diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 95b4dcc31f50fe..30e1509fe1f112 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -6788,7 +6788,7 @@ class Compiler bool IsConstantInt32Assertion() { - return (assertionKind == OAK_EQUAL) && (op2.kind == O2K_CONST_INT); + return ((assertionKind == OAK_EQUAL) || (assertionKind == OAK_NOT_EQUAL)) && (op2.kind == O2K_CONST_INT); } static bool SameKind(AssertionDsc* a1, AssertionDsc* a2) diff --git a/src/coreclr/src/jit/rangecheck.cpp b/src/coreclr/src/jit/rangecheck.cpp index 4401ca426276c6..318ae942a558f6 100644 --- a/src/coreclr/src/jit/rangecheck.cpp +++ b/src/coreclr/src/jit/rangecheck.cpp @@ -565,6 +565,7 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse Limit limit(Limit::keUndef); genTreeOps cmpOper = GT_NONE; + bool isConstantAssertion = false; // Current assertion is of the form (i < len - cns) != 0 if (curAssertion->IsCheckedBoundArithBound()) @@ -644,8 +645,21 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse continue; } - limit = Limit(Limit::keConstant, m_pCompiler->vnStore->ConstantValue(curAssertion->op2.vn)); - cmpOper = GT_EQ; + int cnstLimit = m_pCompiler->vnStore->ConstantValue(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 + { + limit = Limit(Limit::keConstant, cnstLimit); + cmpOper = GT_EQ; + } + + isConstantAssertion = true; } // Current assertion is not supported, ignore it else @@ -656,7 +670,7 @@ 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 ((curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)) && (cmpOper != GT_EQ)) + if ((curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)) && !isConstantAssertion) { continue; } @@ -705,7 +719,7 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse // If we have an 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) && (cmpOper != GT_EQ)) + if ((curAssertion->assertionKind == Compiler::OAK_EQUAL) && !isConstantAssertion) { cmpOper = GenTree::ReverseRelop(cmpOper); } diff --git a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs index bc4016282c495b..b6f61e53f5a83e 100644 --- a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs +++ b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs @@ -189,6 +189,24 @@ public static void NotEqualsOutOfBound(int[] arr) } } + public static void ZeroInBounds(int[] arr) + { + if (arr.Length != 0) + { + arr[0] = 0; + } + } + + public static void ZeroOutOfBounds(int[] arr) + { + if (arr.Length != 0) + { + arr[1] = 0; + } + + return; + } + [MethodImplAttribute(MethodImplOptions.NoInlining)] public static void ModInBounds(int[] arr, uint i) { @@ -329,4 +347,44 @@ public static void EqualsOutOfBound(int[] arr) arr[6] = 1; } + + public static void ZeroInBounds(int[] arr) + { + if (arr.Length == 0) + { + return; + } + + arr[0] = 0; + } + + public static void ZeroOutOfBounds(int[] arr) + { + if (arr.Length == 0) + { + arr[0] = 0; + } + + return; + } + + public static void ZeroInBounds(int[] arr) + { + if (arr.Length == 0) + { + return; + } + + arr[0] = 0; + } + + public static void ZeroOutOfBounds(int[] arr) + { + if (arr.Length != 0) + { + return; + } + + arr[1] = 0; + } } \ No newline at end of file From cd13bfc8423d13be437b06da4c920afcf355e72b Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Fri, 4 Sep 2020 13:53:46 -0400 Subject: [PATCH 07/14] Revert "Revert string changes" This reverts commit 6f77bf8c8acce1f5382bb704875384c6f8e2f984. --- src/libraries/System.Private.CoreLib/src/System/String.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.cs b/src/libraries/System.Private.CoreLib/src/System/String.cs index 49af25beb49c3b..510c1fe97f1263 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.cs @@ -487,13 +487,9 @@ public char[] ToCharArray(int startIndex, int length) [NonVersionable] public static bool IsNullOrEmpty([NotNullWhen(false)] string? value) { - // Using 0u >= (uint)value.Length rather than - // value.Length == 0 as it will elide the bounds check to - // the first char: value[0] if that is performed following the test - // for the same test cost. // Ternary operator returning true/false prevents redundant asm generation: // https://github.com/dotnet/runtime/issues/4207 - return (value == null || 0u >= (uint)value.Length) ? true : false; + return (value == null || 0 == value.Length) ? true : false; } public static bool IsNullOrWhiteSpace([NotNullWhen(false)] string? value) @@ -611,6 +607,7 @@ public StringRuneEnumerator EnumerateRunes() internal static unsafe int wcslen(char* ptr) { // IndexOf processes memory in aligned chunks, and thus it won't crash even if it accesses memory beyond the null terminator. + // This IndexOf behavior is an implementation detail of the runtime and callers outside System.Private.CoreLib must not depend on it. int length = SpanHelpers.IndexOf(ref *ptr, '\0', int.MaxValue); if (length < 0) { @@ -624,6 +621,7 @@ internal static unsafe int wcslen(char* ptr) internal static unsafe int strlen(byte* ptr) { // IndexOf processes memory in aligned chunks, and thus it won't crash even if it accesses memory beyond the null terminator. + // This IndexOf behavior is an implementation detail of the runtime and callers outside System.Private.CoreLib must not depend on it. int length = SpanHelpers.IndexOf(ref *ptr, (byte)'\0', int.MaxValue); if (length < 0) { From 4e0e9f75ae384569f97906247276f72cbdc8c486 Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Fri, 4 Sep 2020 14:03:32 -0400 Subject: [PATCH 08/14] Fix up tests --- src/coreclr/src/jit/assertionprop.cpp | 6 +- src/coreclr/src/jit/rangecheck.cpp | 11 +-- .../AssertionPropagation/ArrBoundMinLength.cs | 78 ++++++++----------- 3 files changed, 42 insertions(+), 53 deletions(-) diff --git a/src/coreclr/src/jit/assertionprop.cpp b/src/coreclr/src/jit/assertionprop.cpp index b554d44416addb..2d0f065f91345e 100644 --- a/src/coreclr/src/jit/assertionprop.cpp +++ b/src/coreclr/src/jit/assertionprop.cpp @@ -1976,12 +1976,12 @@ AssertionInfo Compiler::optAssertionGenJtrue(GenTree* tree) if (con == 0) { dsc.assertionKind = OAK_NOT_EQUAL; - dsc.op1.bnd.vnIdx = vnStore->VNForIntCon(0); + dsc.op1.bnd.vnIdx = vnStore->VNForIntCon(0); } - else + else { dsc.assertionKind = OAK_EQUAL; - dsc.op1.bnd.vnIdx = vnStore->VNForIntCon(con - 1); + dsc.op1.bnd.vnIdx = vnStore->VNForIntCon(con - 1); } dsc.op1.vn = op1VN; diff --git a/src/coreclr/src/jit/rangecheck.cpp b/src/coreclr/src/jit/rangecheck.cpp index 318ae942a558f6..8a5b9d5abe554a 100644 --- a/src/coreclr/src/jit/rangecheck.cpp +++ b/src/coreclr/src/jit/rangecheck.cpp @@ -564,8 +564,8 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse Compiler::AssertionDsc* curAssertion = m_pCompiler->optGetAssertion(assertionIndex); Limit limit(Limit::keUndef); - genTreeOps cmpOper = GT_NONE; - bool isConstantAssertion = false; + genTreeOps cmpOper = GT_NONE; + bool isConstantAssertion = false; // Current assertion is of the form (i < len - cns) != 0 if (curAssertion->IsCheckedBoundArithBound()) @@ -646,11 +646,12 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse } int cnstLimit = m_pCompiler->vnStore->ConstantValue(curAssertion->op2.vn); - - if ((cnstLimit == 0) && (curAssertion->assertionKind == Compiler::OAK_NOT_EQUAL) && m_pCompiler->vnStore->IsVNCheckedBound(curAssertion->op1.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); + limit = Limit(Limit::keConstant, 1); cmpOper = GT_GE; } else diff --git a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs index b6f61e53f5a83e..beeb231582f4a2 100644 --- a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs +++ b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs @@ -8,7 +8,7 @@ class Program { private static int returnCode = 100; - private readonly static int[] arr = new int[6]; + private static int[] arr = new int[6]; public static int Main(string[] args) { @@ -33,14 +33,24 @@ public static int Main(string[] args) RunTestNoThrow(Tests.LessEqualsInBound); RunTestNoThrow(Tests.EqualsInBound); RunTestNoThrow(Tests.EqualsReversedInBound); + RunTestNoThrow(Tests.ZeroInBounds); RunTestNoThrow(TestsEarlyReturn.GreaterInBound); RunTestNoThrow(TestsEarlyReturn.GreaterEqualInBound); RunTestNoThrow(TestsEarlyReturn.LessInBound); RunTestNoThrow(TestsEarlyReturn.LessEqualsInBound); RunTestNoThrow(TestsEarlyReturn.NotEqualsInBound); + RunTestNoThrow(TestsEarlyReturn.ZeroInBounds); Tests.ModInBounds(arr, 11); - try { Tests.ModOutOfBounds(arr, 6); returnCode--; } catch {} + try { Tests.ModOutOfBounds(arr, 6); returnCode--; } catch {} + + arr = new int[0]; + RunTestThrows(Tests.ZeroOutOfBounds); + RunTestThrows(TestsEarlyReturn.ZeroOutOfBounds); + + arr = new int[1]; + RunTestThrows(Tests.OneOutOfBounds); + RunTestThrows(Tests.OneEqualsOutOfBounds); return returnCode; } @@ -141,8 +151,6 @@ public static void LessEqualsOutOfBound(int[] arr) public static void EqualsInBound(int[] arr) { - arr[0] = 1; - if (arr.Length == 6) { arr[5] = 1; @@ -151,8 +159,6 @@ public static void EqualsInBound(int[] arr) public static void EqualsReversedInBound(int[] arr) { - arr[0] = 1; - if (6 == arr.Length) { arr[5] = 1; @@ -161,8 +167,6 @@ public static void EqualsReversedInBound(int[] arr) public static void EqualsOutOfBound(int[] arr) { - arr[0] = 1; - if (arr.Length == 6) { arr[6] = 1; @@ -171,8 +175,6 @@ public static void EqualsOutOfBound(int[] arr) public static void EqualsReversedOutOfBound(int[] arr) { - arr[0] = 1; - if (6 == arr.Length) { arr[6] = 1; @@ -181,8 +183,6 @@ public static void EqualsReversedOutOfBound(int[] arr) public static void NotEqualsOutOfBound(int[] arr) { - arr[0] = 1; - if (arr.Length != 5) { arr[6] = 1; @@ -198,20 +198,36 @@ public static void ZeroInBounds(int[] arr) } public static void ZeroOutOfBounds(int[] arr) + { + if (arr.Length == 0) + { + arr[0] = 0; + } + + return; + } + + public static void OneOutOfBounds(int[] arr) { if (arr.Length != 0) { arr[1] = 0; } + } - return; + public static void OneEqualsOutOfBounds(int[] arr) + { + if (arr.Length == 0) + { + return; + } + + arr[1] = 0; } [MethodImplAttribute(MethodImplOptions.NoInlining)] public static void ModInBounds(int[] arr, uint i) { - arr[0] = 1; //needed to signal that arr isn't null - if (arr.Length == 6) { arr[(int)(i % 6)] = 0; @@ -221,8 +237,6 @@ public static void ModInBounds(int[] arr, uint i) [MethodImplAttribute(MethodImplOptions.NoInlining)] public static void ModOutOfBounds(int[] arr, uint i) { - arr[0] = 1; - if (arr.Length == 6) { arr[(int)(i % 7)] = 0; @@ -314,8 +328,6 @@ public static void LessEqualsOutOfBound(int[] arr) public static void NotEqualsInBound(int[] arr) { - arr[0] = 1; - if (arr.Length != 6) { return; @@ -326,8 +338,6 @@ public static void NotEqualsInBound(int[] arr) public static void NotEqualsOutOfBound(int[] arr) { - arr[0] = 1; - if (arr.Length != 6) { return; @@ -338,8 +348,6 @@ public static void NotEqualsOutOfBound(int[] arr) public static void EqualsOutOfBound(int[] arr) { - arr[0] = 1; - if (arr.Length == 5) { return; @@ -358,26 +366,6 @@ public static void ZeroInBounds(int[] arr) arr[0] = 0; } - public static void ZeroOutOfBounds(int[] arr) - { - if (arr.Length == 0) - { - arr[0] = 0; - } - - return; - } - - public static void ZeroInBounds(int[] arr) - { - if (arr.Length == 0) - { - return; - } - - arr[0] = 0; - } - public static void ZeroOutOfBounds(int[] arr) { if (arr.Length != 0) @@ -385,6 +373,6 @@ public static void ZeroOutOfBounds(int[] arr) return; } - arr[1] = 0; + arr[0] = 0; } -} \ No newline at end of file +} From bbea251bdd411a821d9804a809e8e0a049901c68 Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Sun, 6 Sep 2020 17:23:06 -0400 Subject: [PATCH 09/14] reverting lower bound merging as it may be unsound --- src/coreclr/src/jit/rangecheck.h | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/coreclr/src/jit/rangecheck.h b/src/coreclr/src/jit/rangecheck.h index fb12f3e4b743d1..fdca65306a0c60 100644 --- a/src/coreclr/src/jit/rangecheck.h +++ b/src/coreclr/src/jit/rangecheck.h @@ -376,21 +376,6 @@ struct RangeOps result.uLimit = r2hi; } } - if (r1lo.IsBinOpArray() && r2lo.IsBinOpArray() && r1lo.vn == r2lo.vn) - { - result.lLimit = r2lo.GetConstant() < r1lo.GetConstant() ? r2lo : r1lo; - } - - if (r1lo.IsConstant() && r1lo.GetConstant() >= 0 && r2lo.IsBinOpArray() && - r2lo.GetConstant() >= r1lo.GetConstant()) - { - result.lLimit = r1lo; - } - else if (r2lo.IsConstant() && r2lo.GetConstant() >= 0 && r1lo.IsBinOpArray() && - r1lo.GetConstant() >= r2lo.GetConstant()) - { - result.lLimit = r2lo; - } return result; } }; From e750aa9ff6b8404426e5300bde9679629251cd44 Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Sun, 13 Sep 2020 19:39:52 -0400 Subject: [PATCH 10/14] Fix CI exposed bug and add a couple of test cases --- src/coreclr/src/jit/rangecheck.cpp | 2 +- .../AssertionPropagation/ArrBoundMinLength.cs | 19 ++++++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/jit/rangecheck.cpp b/src/coreclr/src/jit/rangecheck.cpp index 8a5b9d5abe554a..94e1247055dc90 100644 --- a/src/coreclr/src/jit/rangecheck.cpp +++ b/src/coreclr/src/jit/rangecheck.cpp @@ -645,7 +645,7 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse continue; } - int cnstLimit = m_pCompiler->vnStore->ConstantValue(curAssertion->op2.vn); + int cnstLimit = m_pCompiler->vnStore->CoercedConstantValue(curAssertion->op2.vn); if ((cnstLimit == 0) && (curAssertion->assertionKind == Compiler::OAK_NOT_EQUAL) && m_pCompiler->vnStore->IsVNCheckedBound(curAssertion->op1.vn)) diff --git a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs index beeb231582f4a2..64a6fa67f7de86 100644 --- a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs +++ b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs @@ -19,6 +19,7 @@ public static int Main(string[] args) RunTestThrows(Tests.EqualsOutOfBound); RunTestThrows(Tests.EqualsReversedOutOfBound); RunTestThrows(Tests.NotEqualsOutOfBound); + RunTestThrows(Tests.NegativeIndexesThrows); RunTestThrows(TestsEarlyReturn.GreaterOutOfBound); RunTestThrows(TestsEarlyReturn.GreaterEqualOutOfBound); RunTestThrows(TestsEarlyReturn.LessOutOfBound); @@ -33,7 +34,8 @@ public static int Main(string[] args) RunTestNoThrow(Tests.LessEqualsInBound); RunTestNoThrow(Tests.EqualsInBound); RunTestNoThrow(Tests.EqualsReversedInBound); - RunTestNoThrow(Tests.ZeroInBounds); + RunTestNoThrow(Tests.ZeroInBounds); + RunTestNoThrow(Tests.CompareAgainstLong); RunTestNoThrow(TestsEarlyReturn.GreaterInBound); RunTestNoThrow(TestsEarlyReturn.GreaterEqualInBound); RunTestNoThrow(TestsEarlyReturn.LessInBound); @@ -242,6 +244,21 @@ public static void ModOutOfBounds(int[] arr, uint i) arr[(int)(i % 7)] = 0; } } + + public static void CompareAgainstLong(int[] arr) + { + long len = (1L << 32) + 1; + if (arr.Length == len) + arr[0] = 0; + } + + public static void NegativeIndexesThrows(int[] arr) + { + if (arr.Length < 7) + { + arr[-1] = 0; + } + } } public static class TestsEarlyReturn From f7905df1ada22c5181791fef824951d5dc65d470 Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Tue, 15 Sep 2020 15:31:10 -0400 Subject: [PATCH 11/14] code review feedback --- src/coreclr/src/jit/assertionprop.cpp | 12 ++- src/coreclr/src/jit/rangecheck.cpp | 45 +++++++-- src/coreclr/src/jit/rangecheck.h | 4 +- .../AssertionPropagation/ArrBoundMinLength.cs | 97 ++++++++++++++++++- .../ArrBoundMinLength.csproj | 2 +- 5 files changed, 149 insertions(+), 11 deletions(-) diff --git a/src/coreclr/src/jit/assertionprop.cpp b/src/coreclr/src/jit/assertionprop.cpp index 2d0f065f91345e..51973859e09cc7 100644 --- a/src/coreclr/src/jit/assertionprop.cpp +++ b/src/coreclr/src/jit/assertionprop.cpp @@ -1969,10 +1969,15 @@ AssertionInfo Compiler::optAssertionGenJtrue(GenTree* tree) } else if (vnStore->IsVNCheckedBound(op1VN) && vnStore->IsVNInt32Constant(op2VN)) { + assert(relop->OperIs(GT_EQ, GT_NE)); + int con = vnStore->ConstantValue(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; @@ -1992,12 +1997,17 @@ AssertionInfo Compiler::optAssertionGenJtrue(GenTree* tree) 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); } - return index; + else + { + return index; + } } } diff --git a/src/coreclr/src/jit/rangecheck.cpp b/src/coreclr/src/jit/rangecheck.cpp index 94e1247055dc90..960807faf8ef6e 100644 --- a/src/coreclr/src/jit/rangecheck.cpp +++ b/src/coreclr/src/jit/rangecheck.cpp @@ -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, int arrSize) +//------------------------------------------------------------------------ +// BetweenBounds: Check if the computed range is within bounds +// +// Arguments: +// Range - the computed range of upper +// 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"); } @@ -212,9 +228,11 @@ 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) { - // see if there are any assertions about the array size we can use JITDUMP("Looking for array size assertions for: " FMT_VN "\n", arrLenVn); Range arrLength = Range(Limit(Limit::keDependent)); MergeEdgeAssertions(arrLenVn, block->bbAssertionIn, &arrLength); @@ -278,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, arrSize)) + if (BetweenBounds(range, bndsChk->gtArrLen, arrSize)) { JITDUMP("[RangeCheck::OptimizeRangeCheck] Between bounds\n"); m_pCompiler->optRemoveRangeCheck(treeParent, stmt); @@ -524,6 +542,14 @@ void RangeCheck::SetDef(UINT64 hash, Location* loc) } #endif +//------------------------------------------------------------------------ +// 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) @@ -541,7 +567,14 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP MergeEdgeAssertions(normalLclVN, assertions, pRange); } -// 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: +// 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)) diff --git a/src/coreclr/src/jit/rangecheck.h b/src/coreclr/src/jit/rangecheck.h index fdca65306a0c60..751b243740c408 100644 --- a/src/coreclr/src/jit/rangecheck.h +++ b/src/coreclr/src/jit/rangecheck.h @@ -434,11 +434,11 @@ class RangeCheck int GetArrLength(ValueNum vn); - // Check whether the computed range is within lower and upper bounds. This function + // Check whether the computed range is within 0 and upper bounds. 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 BetweenBounds(Range& range, int lower, GenTree* upper, int arrSize); + bool BetweenBounds(Range& range, GenTree* upper, int arrSize); // Entry point to optimize range checks in the block. Assumes value numbering // and assertion prop phases are completed. diff --git a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs index 64a6fa67f7de86..54c119ba7a45e8 100644 --- a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs +++ b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.cs @@ -42,6 +42,9 @@ public static int Main(string[] args) RunTestNoThrow(TestsEarlyReturn.LessEqualsInBound); RunTestNoThrow(TestsEarlyReturn.NotEqualsInBound); RunTestNoThrow(TestsEarlyReturn.ZeroInBounds); + RunTestNoThrow((int[] arr) => Tests.EqualsAgainstBoundFiveIndex(arr, 6)); + RunTestNoThrow((int[] arr) => TestsEarlyReturn.NotEqualsAgainstBoundFiveIndex(arr, 6)); + Tests.ModInBounds(arr, 11); try { Tests.ModOutOfBounds(arr, 6); returnCode--; } catch {} @@ -49,10 +52,22 @@ public static int Main(string[] args) arr = new int[0]; RunTestThrows(Tests.ZeroOutOfBounds); RunTestThrows(TestsEarlyReturn.ZeroOutOfBounds); + RunTestThrows((int[] arr) => Tests.EqualsAgainstBoundZeroIndex(arr, 0)); + RunTestThrows((int[] arr) => TestsEarlyReturn.EqualsAgainstBoundZeroIndexOutOfBound(arr, 1)); + RunTestThrows((int[] arr) => TestsEarlyReturn.EqualsAgainstBoundZeroIndex(arr, 0)); + RunTestThrows((int[] arr) => TestsEarlyReturn.NotEqualsAgainstBoundFiveIndex(arr, 0)); + RunTestThrows((int[] arr) => Tests.NotEqualsAgainstBoundZeroIndex(arr, 1)); + RunTestNoThrow((int[] arr) => TestsEarlyReturn.NotEqualsAgainstBoundFiveIndex(arr, 6)); arr = new int[1]; RunTestThrows(Tests.OneOutOfBounds); RunTestThrows(Tests.OneEqualsOutOfBounds); + RunTestThrows((int[] arr) => TestsEarlyReturn.EqualsFiveIndexOutOfBound(arr, 6)); + RunTestThrows((int[] arr) => Tests.NotEqualsAgainstBoundFiveIndex(arr, 0)); + RunTestThrows((int[] arr) => Tests.NotEqualsAgainstBoundFiveIndex(arr, 5)); + RunTestNoThrow((int[] arr) => TestsEarlyReturn.EqualsAgainstBoundZeroIndex(arr, 1)); + RunTestNoThrow((int[] arr) => Tests.EqualsAgainstBoundZeroIndex(arr, 1)); + RunTestNoThrow((int[] arr) => Tests.NotEqualsAgainstBoundZeroIndex(arr, 0)); return returnCode; } @@ -254,11 +269,47 @@ public static void CompareAgainstLong(int[] arr) public static void NegativeIndexesThrows(int[] arr) { - if (arr.Length < 7) + if (arr.Length > 5) { arr[-1] = 0; } } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static void EqualsAgainstBoundFiveIndex(int[] arr, int bound) + { + if (arr.Length == bound) + { + arr[5] = 1; + } + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static void NotEqualsAgainstBoundFiveIndex(int[] arr, int bound) + { + if (arr.Length != bound) + { + arr[5] = 1; + } + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static void EqualsAgainstBoundZeroIndex(int[] arr, int bound) + { + if (arr.Length == bound) + { + arr[0] = 1; + } + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static void NotEqualsAgainstBoundZeroIndex(int[] arr, int bound) + { + if (arr.Length != bound) + { + arr[0] = 1; + } + } } public static class TestsEarlyReturn @@ -392,4 +443,48 @@ public static void ZeroOutOfBounds(int[] arr) arr[0] = 0; } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static void NotEqualsAgainstBoundFiveIndex(int[] arr, int bound) + { + if (arr.Length != bound) + { + return; + } + + arr[5] = 1; + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static void EqualsAgainstBoundZeroIndex(int[] arr, int bound) + { + if (arr.Length != bound) + { + return; + } + + arr[0] = 1; + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static void EqualsFiveIndexOutOfBound(int[] arr, int bound) + { + if (arr.Length == bound) + { + return; + } + + arr[5] = 1; + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static void EqualsAgainstBoundZeroIndexOutOfBound(int[] arr, int bound) + { + if (arr.Length == bound) + { + return; + } + + arr[0] = 1; + } } diff --git a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.csproj b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.csproj index 5c20dcadd57cac..f3e1cbd44b4041 100644 --- a/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.csproj +++ b/src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.csproj @@ -7,6 +7,6 @@ True - + From 357a10700be1a249442ee497844e890b283f0ffc Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Tue, 15 Sep 2020 15:57:10 -0400 Subject: [PATCH 12/14] comment nit --- src/coreclr/src/jit/rangecheck.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/src/jit/rangecheck.cpp b/src/coreclr/src/jit/rangecheck.cpp index 960807faf8ef6e..df9f12d80fe3e0 100644 --- a/src/coreclr/src/jit/rangecheck.cpp +++ b/src/coreclr/src/jit/rangecheck.cpp @@ -63,7 +63,7 @@ int RangeCheck::GetArrLength(ValueNum vn) // BetweenBounds: Check if the computed range is within bounds // // Arguments: -// Range - the computed range of upper +// Range - the range to check if in bounds // upper - the array length vn // arrSize - the length of the array if known, or <= 0 // From cac0180821806f139a35c07e63698e690e049913 Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Mon, 5 Oct 2020 19:19:28 -0400 Subject: [PATCH 13/14] feedback --- src/coreclr/src/jit/rangecheck.cpp | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/coreclr/src/jit/rangecheck.cpp b/src/coreclr/src/jit/rangecheck.cpp index df9f12d80fe3e0..b6eb07eec029ef 100644 --- a/src/coreclr/src/jit/rangecheck.cpp +++ b/src/coreclr/src/jit/rangecheck.cpp @@ -103,7 +103,7 @@ bool RangeCheck::BetweenBounds(Range& range, GenTree* upper, int arrSize) if ((arrSize <= 0) && !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; } @@ -687,11 +687,16 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse limit = Limit(Limit::keConstant, 1); cmpOper = GT_GE; } - else + 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; } @@ -703,8 +708,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 ((curAssertion->op2.vn != m_pCompiler->vnStore->VNZeroForType(TYP_INT)) && !isConstantAssertion) + // 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; } @@ -744,13 +749,13 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse // (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) && !isConstantAssertion) @@ -758,6 +763,8 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse cmpOper = GenTree::ReverseRelop(cmpOper); } + 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)) { From 1ee8edac92fca794ef784d35948ed44f31cae636 Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Mon, 5 Oct 2020 22:56:11 -0400 Subject: [PATCH 14/14] Add missing break --- src/coreclr/src/jit/rangecheck.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/src/jit/rangecheck.cpp b/src/coreclr/src/jit/rangecheck.cpp index b6eb07eec029ef..3b5c925229bd30 100644 --- a/src/coreclr/src/jit/rangecheck.cpp +++ b/src/coreclr/src/jit/rangecheck.cpp @@ -834,6 +834,7 @@ void RangeCheck::MergeEdgeAssertions(ValueNum normalLclVN, ASSERT_VALARG_TP asse case GT_EQ: pRange->uLimit = limit; pRange->lLimit = limit; + break; default: // All other 'cmpOper' kinds leave lLimit/uLimit unchanged