From 72135b8c65f12ad8477eea0c9cb29e03b058125c Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 18 Sep 2026 14:58:55 +0200 Subject: [PATCH 1/2] JIT: require nonnegative provenance for checked bounds Do not treat method-wide checked-bound membership as proof that a value is nonnegative on every path. Validate symbolic bound bases centrally and avoid creating no-throw assertions from unsigned comparisons without nonnegative provenance. Preserve same-length unsigned bounds-check elimination and prove span slice lengths from incoming comparisons rather than checked-bound metadata. Add a source-only regression covering signed, reversed, offset, equality, unsigned, and subtraction boundary cases. Validation: 55,271 benchmarks.run. contexts replay cleanly; asmdiffs are +61 bytes total (-50/+111), with a largest method regression of 56 bytes. The 120-test recent regression subset and normal/stress/repeated-optimization regression runs pass. Fixes #133821 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a79fe91-9a35-47c4-aa46-995ddd867f58 --- src/coreclr/jit/assertionprop.cpp | 18 ++- src/coreclr/jit/compiler.h | 8 +- src/coreclr/jit/rangecheck.cpp | 69 +++++++-- src/coreclr/jit/rangecheck.h | 4 +- src/coreclr/jit/valuenum.cpp | 2 +- .../JIT/Regression_ro_2/Runtime_133821.cs | 138 ++++++++++++++++++ 6 files changed, 212 insertions(+), 27 deletions(-) create mode 100644 src/tests/JIT/Regression_ro_2/Runtime_133821.cs diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index f50d2f09bcb861..0ce66e929db7fd 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -1903,8 +1903,10 @@ AssertionInfo Compiler::optCreateJTrueBoundsAssertion(GenTree* tree) } } + // Also track start u<= length (and its complement) to prove span slice lengths non-negative. if (!isUnsignedCompareCheckedBound && isUnsignedRelop && (op1VN != op2VN) && !vnStore->IsVNConstant(op1VN) && - !vnStore->IsVNConstant(op2VN) && vnStore->IsVNCheckedBoundIndex(op1VN) && optAssertionHasAssertionsForVN(op2VN)) + !vnStore->IsVNConstant(op2VN) && optAssertionHasAssertionsForVN(op2VN) && + (vnStore->IsVNCheckedBoundIndex(op1VN) || relopFuncApp.FuncIs(VNF_GT_UN, VNF_LE_UN))) { AssertionDsc dsc = AssertionDsc::CreateRelopVN(this, relopFunc, op1VN, op2VN); AssertionIndex idx = optAddAssertion(dsc); @@ -1943,14 +1945,18 @@ AssertionInfo Compiler::optCreateJTrueBoundsAssertion(GenTree* tree) return idx; } - // Loop condition like "(uint)i < (uint)bnd" or equivalent - // Assertion: "no throw" since this condition guarantees that i is both >= 0 and < bnd (on the appropriate edge) + // Loop condition like "(uint)i < (uint)bnd" or equivalent. + // This only implies a no-throw bounds check if bnd is known to be non-negative at this comparison. if (isUnsignedCompareCheckedBound) { ValueNum idxVN = vnStore->VNNormalValue(unsignedCompareBnd.vnIdx); ValueNum lenVN = vnStore->VNNormalValue(unsignedCompareBnd.vnBound); - AssertionDsc dsc = AssertionDsc::CreateNoThrowArrBnd(this, idxVN, lenVN); + bool isLenNeverNegative = + ((optConservativeNormalVN(relop->gtGetOp1()) == lenVN) && relop->gtGetOp1()->IsNeverNegative(this)) || + ((optConservativeNormalVN(relop->gtGetOp2()) == lenVN) && relop->gtGetOp2()->IsNeverNegative(this)); + AssertionDsc dsc = + AssertionDsc::CreateCompareCheckedBound(this, VNF_LT_UN, idxVN, lenVN, 0, isLenNeverNegative); AssertionIndex index = optAddAssertion(dsc); if (unsignedCompareBnd.cmpOper == VNF_GE_UN) { @@ -5706,13 +5712,13 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, { // If it is not a nothrow assertion, skip. const AssertionDsc& curAssertion = optGetAssertion(GetAssertionIndex(index)); - if (!curAssertion.IsBoundsCheckNoThrow()) + if (!curAssertion.IsBoundsCheckNoThrow(vnCurLen)) { continue; } assert(curAssertion.GetOp2().GetCns() == 0); - assert(curAssertion.GetOp2().IsVNNeverNegative()); + assert(curAssertion.GetOp2().IsVNNeverNegative() || (curAssertion.GetOp2().GetVN() == vnCurLen)); // Do we have a previous range check involving the same 'vnLen' upper bound? if (curAssertion.GetOp2().GetVN() == vnCurLen) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 8df148e859f87e..f78be3b571284a 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -8758,12 +8758,12 @@ class Compiler return false; } - bool IsBoundsCheckNoThrow() const + bool IsBoundsCheckNoThrow(ValueNum checkedBoundVN) const { - // O1K_VN (idx) u< O2K_VN_ADD_CNS (len) where len is never negative. - // Effectively, it's "idx >= 0 && idx < len" + // O1K_VN (idx) u< O2K_VN_ADD_CNS (len) where len is known non-negative, + // either from the assertion or because it is the bound being checked. return GetOp1().KindIs(O1K_VN) && KindIs(OAK_LT_UN) && GetOp2().KindIs(O2K_VN_ADD_CNS) && - (GetOp2().GetCns() == 0) && GetOp2().IsVNNeverNegative(); + (GetOp2().GetCns() == 0) && (GetOp2().IsVNNeverNegative() || (GetOp2().GetVN() == checkedBoundVN)); } // Convert VNFunc to optAssertionKind diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 7b2a38a7b5171e..98a6c782510ad5 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -749,8 +749,26 @@ Range RangeCheck::GetRangeFromAssertionsWorker( binOpResult = RangeOps::Multiply(r1, r2); break; case VNF_SUB: + { binOpResult = RangeOps::Subtract(r1, r2); + // Preserve the non-negativity of span slice lengths: length - start, + // where length >= 0 and an incoming unsigned comparison proves start <= length. + if ((vnType == TYP_INT) && r1.IsConstantRange() && (r1.LowerLimit().GetConstant() >= 0) && + (!binOpResult.LowerLimit().IsConstant() || (binOpResult.LowerLimit().GetConstant() < 0))) + { + Range startRange = Limit(Limit::keUnknown); + MergeEdgeAssertionsWorker(comp, funcApp.GetArg(1), funcApp.GetArg(0), assertions, + &startRange, true, budget - 1, visited); + if (startRange.LowerLimit().IsConstant() && (startRange.LowerLimit().GetConstant() >= 0) && + startRange.UpperLimit().IsBinOpArray() && + (startRange.UpperLimit().vn == funcApp.GetArg(0)) && + (startRange.UpperLimit().GetConstant() <= 0)) + { + binOpResult = Range(Limit(Limit::keConstant, 0), r1.UpperLimit()); + } + } break; + } case VNF_AND: binOpResult = RangeOps::And(r1, r2); break; @@ -1112,7 +1130,7 @@ Limit RangeCheck::TightenLimit(Limit l1, Limit l2, ValueNum preferredBound, bool // Arguments: // comp - the compiler instance // normalLclVN - the value number to look for assertions for -// preferredBoundVN - when this VN is set, it will be given preference over constant limits +// preferredBoundVN - a bound known non-negative at the query site, preferred over constant limits // assertions - the assertions to use // pRange - the range to tighten with assertions // canUseCheckedBounds - true if we can use checked bounds assertions (cache) @@ -1138,7 +1156,7 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp, // Arguments: // comp - the compiler instance // normalLclVN - the value number to look for assertions for -// preferredBoundVN - when this VN is set, it will be given preference over constant limits +// preferredBoundVN - a bound known non-negative at the query site, preferred over constant limits // assertions - the assertions to use // pRange - the range to tighten with assertions // canUseCheckedBounds - true if we can use checked bounds assertions (cache) @@ -1263,18 +1281,16 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler* comp // Current assertion is "normalLclVN u<= preferredBoundVN". else if (canUseCheckedBounds && curAssertion.KindIs(Compiler::OAK_LE_UN) && (curAssertion.GetOp1().GetVN() == normalLclVN) && - curAssertion.GetOp2().KindIs(Compiler::O2K_VN_ADD_CNS) && curAssertion.GetOp2().IsVNNeverNegative() && + curAssertion.GetOp2().KindIs(Compiler::O2K_VN_ADD_CNS) && (curAssertion.GetOp2().GetVN() == preferredBoundVN) && (curAssertion.GetOp2().GetCns() == 0)) { cmpOper = GT_LE; limit = Limit(Limit::keBinOpArray, preferredBoundVN, 0); isUnsigned = true; } - // Current assertion is of the form "i (vn + cns)" where vn is a real - // (length-like) checked bound. The arbitrary-VN sub-form of O2K_VN_ADD_CNS (created by - // CreateRelopVN, where op2.vn is not a checked bound) is intentionally excluded here so - // that it never flows into a keBinOpArray Limit, whose Range::IsValid / Range::Widen - // rules implicitly assume the VN refers to a length-like (checked-bound-shaped) quantity. + // Current assertion is of the form "i (vn + cns)" where vn is a checked bound. + // The arbitrary-VN sub-form of O2K_VN_ADD_CNS (created by CreateRelopVN, where op2.vn + // is not a checked bound) is intentionally excluded here. // Such cases fall through to the general "X Y" branch below. else if (curAssertion.KindIs(Compiler::OAK_GE, Compiler::OAK_GT, Compiler::OAK_LE, Compiler::OAK_LT) && curAssertion.GetOp2().KindIs(Compiler::O2K_VN_ADD_CNS) && @@ -1437,15 +1453,25 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler* comp } } } - // Current assertion asserts a bounds check does not throw - else if (curAssertion.IsBoundsCheckNoThrow()) + // Current assertion is "index u< length". It implies a no-throw bounds check + // only when length is non-negative at this site. + else if (curAssertion.IsBoundsCheckNoThrow(preferredBoundVN) || + (curAssertion.KindIs(Compiler::OAK_LT_UN) && curAssertion.GetOp2().KindIs(Compiler::O2K_VN_ADD_CNS) && + (curAssertion.GetOp2().GetCns() == 0) && (curAssertion.GetOp2().GetVN() == normalLclVN))) { - // IsBoundsCheckNoThrow is "op1VN (Idx) LT_UN op2VN (Len)" ValueNum indexVN = curAssertion.GetOp1().GetVN(); ValueNum lenVN = curAssertion.GetOp2().GetVN(); - assert(curAssertion.GetOp2().GetCns() == 0); - assert(curAssertion.GetOp2().IsVNNeverNegative()); + if (!curAssertion.GetOp2().IsVNNeverNegative() && (lenVN != preferredBoundVN) && + !((normalLclVN == lenVN) && pRange->LowerLimit().IsConstant() && + (pRange->LowerLimit().GetConstant() >= 0))) + { + Range boundRange = GetRangeFromAssertionsWorker(comp, lenVN, assertions, budget - 1, visited); + if (!boundRange.LowerLimit().IsConstant() || (boundRange.LowerLimit().GetConstant() < 0)) + { + continue; + } + } if (normalLclVN == indexVN) { @@ -1617,6 +1643,21 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler* comp } assert(limit.IsBinOpArray() || limit.IsConstant()); + + // Symbolic limits require a non-negative base, not merely a VN used in some bounds check. + // The preferred bound is already known non-negative at the query site. Other bounds + // need independent proof: new int[n] on another path does not imply n >= 0 here. + if (limit.IsBinOpArray() && (limit.vn != preferredBoundVN) && + !((limit.vn == curAssertion.GetOp2().GetVN()) && curAssertion.GetOp2().IsVNNeverNegative()) && + !comp->vnStore->IsVNNeverNegative(limit.vn)) + { + Range boundRange = GetRangeFromAssertionsWorker(comp, limit.vn, assertions, budget, visited); + if (!boundRange.LowerLimit().IsConstant() || (boundRange.LowerLimit().GetConstant() < 0)) + { + continue; + } + } + #ifdef DEBUG if (comp->verbose) { @@ -2416,7 +2457,7 @@ void Indent(int indent) // block - the block that contains `expr`; // expr - expression to compute the range for; // pRange - [Out] range of the expression; -// preferredBoundVN - a value number of the preferred bound. +// preferredBoundVN - a bound known non-negative at the query site, preferred over constant limits. // // Return Value: // false if the range is unknown or determined to overflow. diff --git a/src/coreclr/jit/rangecheck.h b/src/coreclr/jit/rangecheck.h index 64c38ac7b687b9..ce0ae9ff630569 100644 --- a/src/coreclr/jit/rangecheck.h +++ b/src/coreclr/jit/rangecheck.h @@ -76,8 +76,8 @@ struct Limit { enum LimitType { - keUndef, // The limit is yet to be computed. - keBinOpArray, + keUndef, // The limit is yet to be computed. + keBinOpArray, // A non-negative bound VN plus a constant. keConstant, keDependent, // The limit is dependent on some other value. keUnknown, // The limit could not be determined. diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 51ac48dbd8516a..e3ffd47619bda3 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -7918,7 +7918,7 @@ bool ValueNumStore::IsVNCheckedBound(ValueNum vn) if (m_checkedBoundVNs.TryGetValue(vn, &dummy)) { // This VN appeared as the conservative VN of the length argument of some - // GT_BOUNDS_CHECK node. + // GT_BOUNDS_CHECK node. This does not imply the VN is non-negative on other paths. return true; } if (IsVNArrLen(vn)) diff --git a/src/tests/JIT/Regression_ro_2/Runtime_133821.cs b/src/tests/JIT/Regression_ro_2/Runtime_133821.cs new file mode 100644 index 00000000000000..5704872135bf23 --- /dev/null +++ b/src/tests/JIT/Regression_ro_2/Runtime_133821.cs @@ -0,0 +1,138 @@ +// 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; +using Xunit; + +public class Runtime_133821 +{ + private static int s_sink; + + [Fact] + public static void TestEntryPoint() + { + int[] arr = { 10, 11, 12, 13, 14, 15, 16, 17 }; + Assert.Equal(10, Signed(arr, 4, 2, true)); + Assert.Equal(14, Signed(arr, 4, 4, false)); + Assert.Equal(-1, Signed(arr, 4, 8, false)); + Assert.Throws(() => Signed(arr, -5, -3, false)); + Assert.Throws(() => SignedOffset(arr, -5, -3, false)); + Assert.Throws(() => SignedReversed(arr, -5, -3, false)); + Assert.Throws(() => Equal(arr, -3, -3, false)); + Assert.Throws(() => Unsigned(arr, -3, -5, false)); + + Assert.Equal(0, Subtract(0, 0)); + Assert.Equal(7, Subtract(10, 3)); + Assert.Equal(int.MaxValue, Subtract(int.MaxValue, 0)); + Assert.Equal(0, Subtract(int.MaxValue, int.MaxValue)); + Assert.Equal(-1, Subtract(10, 11)); + Assert.Equal(-1, Subtract(10, -1)); + Assert.Equal(-1, Subtract(int.MaxValue, int.MinValue)); + Assert.Equal(-1, Subtract(int.MinValue, int.MaxValue)); + } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static int Signed(int[] arr, int n, int k, bool cond) + { + if (cond) + { + int[] tmp = new int[n]; + s_sink = tmp[0]; + } + + int i = 0; + if (k >= n) + { + i = k; + } + + return i < arr.Length ? arr[i] : -1; + } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static int SignedOffset(int[] arr, int n, int k, bool cond) + { + if (cond) + { + int[] tmp = new int[n]; + s_sink = tmp[0]; + } + + int i = 0; + if (k >= n - 1) + { + i = k + 1; + } + + return i < arr.Length ? arr[i] : -1; + } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static int SignedReversed(int[] arr, int n, int k, bool cond) + { + if (cond) + { + int[] tmp = new int[n]; + s_sink = tmp[0]; + int[] other = new int[k]; + s_sink = other[0]; + } + + int i = 0; + if (k >= n) + { + i = k; + } + + return i < arr.Length ? arr[i] : -1; + } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static int Equal(int[] arr, int n, int k, bool cond) + { + if (cond) + { + int[] tmp = new int[n]; + s_sink = tmp[0]; + } + + int i = 0; + if (k == n) + { + i = k; + } + + return i < arr.Length ? arr[i] : -1; + } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static int Unsigned(int[] arr, int n, int k, bool cond) + { + if (cond) + { + int[] tmp = new int[n]; + s_sink = tmp[0]; + } + + int i = 0; + if ((uint)k < (uint)n) + { + i = k; + } + + return i < arr.Length ? arr[i] : -1; + } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static int Subtract(int length, int start) + { + if (length < 0 || (uint)start > (uint)length) + { + return -1; + } + + int remaining = length - start; + return remaining < 0 ? -2 : remaining; + } +} From 3b65cfccaff54b076c18d78168c880b3863168f3 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 18 Sep 2026 15:08:12 +0200 Subject: [PATCH 2/2] Narrow checked-bound fix to the reported signed comparison bug Keep only the shared symbolic-bound nonnegativity guard and the minimal source-only regression. Leave unsigned assertion handling and span-slice analysis for separate work. The complete branch diff now changes two files: 10 JIT lines and a 39-line regression. The test fails on the original JIT and passes with the fix. All 55,271 benchmarks.run. contexts replay cleanly; asmdiffs total +76 bytes across two methods. The 120-test recent regression subset and targeted JitStress=2 run pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a79fe91-9a35-47c4-aa46-995ddd867f58 --- src/coreclr/jit/assertionprop.cpp | 18 +-- src/coreclr/jit/compiler.h | 8 +- src/coreclr/jit/rangecheck.cpp | 65 +++-------- src/coreclr/jit/rangecheck.h | 4 +- src/coreclr/jit/valuenum.cpp | 2 +- .../JIT/Regression_ro_2/Runtime_133821.cs | 109 +----------------- 6 files changed, 35 insertions(+), 171 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 0ce66e929db7fd..f50d2f09bcb861 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -1903,10 +1903,8 @@ AssertionInfo Compiler::optCreateJTrueBoundsAssertion(GenTree* tree) } } - // Also track start u<= length (and its complement) to prove span slice lengths non-negative. if (!isUnsignedCompareCheckedBound && isUnsignedRelop && (op1VN != op2VN) && !vnStore->IsVNConstant(op1VN) && - !vnStore->IsVNConstant(op2VN) && optAssertionHasAssertionsForVN(op2VN) && - (vnStore->IsVNCheckedBoundIndex(op1VN) || relopFuncApp.FuncIs(VNF_GT_UN, VNF_LE_UN))) + !vnStore->IsVNConstant(op2VN) && vnStore->IsVNCheckedBoundIndex(op1VN) && optAssertionHasAssertionsForVN(op2VN)) { AssertionDsc dsc = AssertionDsc::CreateRelopVN(this, relopFunc, op1VN, op2VN); AssertionIndex idx = optAddAssertion(dsc); @@ -1945,18 +1943,14 @@ AssertionInfo Compiler::optCreateJTrueBoundsAssertion(GenTree* tree) return idx; } - // Loop condition like "(uint)i < (uint)bnd" or equivalent. - // This only implies a no-throw bounds check if bnd is known to be non-negative at this comparison. + // Loop condition like "(uint)i < (uint)bnd" or equivalent + // Assertion: "no throw" since this condition guarantees that i is both >= 0 and < bnd (on the appropriate edge) if (isUnsignedCompareCheckedBound) { ValueNum idxVN = vnStore->VNNormalValue(unsignedCompareBnd.vnIdx); ValueNum lenVN = vnStore->VNNormalValue(unsignedCompareBnd.vnBound); - bool isLenNeverNegative = - ((optConservativeNormalVN(relop->gtGetOp1()) == lenVN) && relop->gtGetOp1()->IsNeverNegative(this)) || - ((optConservativeNormalVN(relop->gtGetOp2()) == lenVN) && relop->gtGetOp2()->IsNeverNegative(this)); - AssertionDsc dsc = - AssertionDsc::CreateCompareCheckedBound(this, VNF_LT_UN, idxVN, lenVN, 0, isLenNeverNegative); + AssertionDsc dsc = AssertionDsc::CreateNoThrowArrBnd(this, idxVN, lenVN); AssertionIndex index = optAddAssertion(dsc); if (unsignedCompareBnd.cmpOper == VNF_GE_UN) { @@ -5712,13 +5706,13 @@ GenTree* Compiler::optAssertionProp_BndsChk(ASSERT_VALARG_TP assertions, { // If it is not a nothrow assertion, skip. const AssertionDsc& curAssertion = optGetAssertion(GetAssertionIndex(index)); - if (!curAssertion.IsBoundsCheckNoThrow(vnCurLen)) + if (!curAssertion.IsBoundsCheckNoThrow()) { continue; } assert(curAssertion.GetOp2().GetCns() == 0); - assert(curAssertion.GetOp2().IsVNNeverNegative() || (curAssertion.GetOp2().GetVN() == vnCurLen)); + assert(curAssertion.GetOp2().IsVNNeverNegative()); // Do we have a previous range check involving the same 'vnLen' upper bound? if (curAssertion.GetOp2().GetVN() == vnCurLen) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index f78be3b571284a..8df148e859f87e 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -8758,12 +8758,12 @@ class Compiler return false; } - bool IsBoundsCheckNoThrow(ValueNum checkedBoundVN) const + bool IsBoundsCheckNoThrow() const { - // O1K_VN (idx) u< O2K_VN_ADD_CNS (len) where len is known non-negative, - // either from the assertion or because it is the bound being checked. + // O1K_VN (idx) u< O2K_VN_ADD_CNS (len) where len is never negative. + // Effectively, it's "idx >= 0 && idx < len" return GetOp1().KindIs(O1K_VN) && KindIs(OAK_LT_UN) && GetOp2().KindIs(O2K_VN_ADD_CNS) && - (GetOp2().GetCns() == 0) && (GetOp2().IsVNNeverNegative() || (GetOp2().GetVN() == checkedBoundVN)); + (GetOp2().GetCns() == 0) && GetOp2().IsVNNeverNegative(); } // Convert VNFunc to optAssertionKind diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 98a6c782510ad5..a9b16cee676061 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -749,26 +749,8 @@ Range RangeCheck::GetRangeFromAssertionsWorker( binOpResult = RangeOps::Multiply(r1, r2); break; case VNF_SUB: - { binOpResult = RangeOps::Subtract(r1, r2); - // Preserve the non-negativity of span slice lengths: length - start, - // where length >= 0 and an incoming unsigned comparison proves start <= length. - if ((vnType == TYP_INT) && r1.IsConstantRange() && (r1.LowerLimit().GetConstant() >= 0) && - (!binOpResult.LowerLimit().IsConstant() || (binOpResult.LowerLimit().GetConstant() < 0))) - { - Range startRange = Limit(Limit::keUnknown); - MergeEdgeAssertionsWorker(comp, funcApp.GetArg(1), funcApp.GetArg(0), assertions, - &startRange, true, budget - 1, visited); - if (startRange.LowerLimit().IsConstant() && (startRange.LowerLimit().GetConstant() >= 0) && - startRange.UpperLimit().IsBinOpArray() && - (startRange.UpperLimit().vn == funcApp.GetArg(0)) && - (startRange.UpperLimit().GetConstant() <= 0)) - { - binOpResult = Range(Limit(Limit::keConstant, 0), r1.UpperLimit()); - } - } break; - } case VNF_AND: binOpResult = RangeOps::And(r1, r2); break; @@ -1130,7 +1112,7 @@ Limit RangeCheck::TightenLimit(Limit l1, Limit l2, ValueNum preferredBound, bool // Arguments: // comp - the compiler instance // normalLclVN - the value number to look for assertions for -// preferredBoundVN - a bound known non-negative at the query site, preferred over constant limits +// preferredBoundVN - when this VN is set, it will be given preference over constant limits // assertions - the assertions to use // pRange - the range to tighten with assertions // canUseCheckedBounds - true if we can use checked bounds assertions (cache) @@ -1156,7 +1138,7 @@ void RangeCheck::MergeEdgeAssertions(Compiler* comp, // Arguments: // comp - the compiler instance // normalLclVN - the value number to look for assertions for -// preferredBoundVN - a bound known non-negative at the query site, preferred over constant limits +// preferredBoundVN - when this VN is set, it will be given preference over constant limits // assertions - the assertions to use // pRange - the range to tighten with assertions // canUseCheckedBounds - true if we can use checked bounds assertions (cache) @@ -1281,16 +1263,18 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler* comp // Current assertion is "normalLclVN u<= preferredBoundVN". else if (canUseCheckedBounds && curAssertion.KindIs(Compiler::OAK_LE_UN) && (curAssertion.GetOp1().GetVN() == normalLclVN) && - curAssertion.GetOp2().KindIs(Compiler::O2K_VN_ADD_CNS) && + curAssertion.GetOp2().KindIs(Compiler::O2K_VN_ADD_CNS) && curAssertion.GetOp2().IsVNNeverNegative() && (curAssertion.GetOp2().GetVN() == preferredBoundVN) && (curAssertion.GetOp2().GetCns() == 0)) { cmpOper = GT_LE; limit = Limit(Limit::keBinOpArray, preferredBoundVN, 0); isUnsigned = true; } - // Current assertion is of the form "i (vn + cns)" where vn is a checked bound. - // The arbitrary-VN sub-form of O2K_VN_ADD_CNS (created by CreateRelopVN, where op2.vn - // is not a checked bound) is intentionally excluded here. + // Current assertion is of the form "i (vn + cns)" where vn is a real + // (length-like) checked bound. The arbitrary-VN sub-form of O2K_VN_ADD_CNS (created by + // CreateRelopVN, where op2.vn is not a checked bound) is intentionally excluded here so + // that it never flows into a keBinOpArray Limit, whose Range::IsValid / Range::Widen + // rules implicitly assume the VN refers to a length-like (checked-bound-shaped) quantity. // Such cases fall through to the general "X Y" branch below. else if (curAssertion.KindIs(Compiler::OAK_GE, Compiler::OAK_GT, Compiler::OAK_LE, Compiler::OAK_LT) && curAssertion.GetOp2().KindIs(Compiler::O2K_VN_ADD_CNS) && @@ -1453,25 +1437,15 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler* comp } } } - // Current assertion is "index u< length". It implies a no-throw bounds check - // only when length is non-negative at this site. - else if (curAssertion.IsBoundsCheckNoThrow(preferredBoundVN) || - (curAssertion.KindIs(Compiler::OAK_LT_UN) && curAssertion.GetOp2().KindIs(Compiler::O2K_VN_ADD_CNS) && - (curAssertion.GetOp2().GetCns() == 0) && (curAssertion.GetOp2().GetVN() == normalLclVN))) + // Current assertion asserts a bounds check does not throw + else if (curAssertion.IsBoundsCheckNoThrow()) { + // IsBoundsCheckNoThrow is "op1VN (Idx) LT_UN op2VN (Len)" ValueNum indexVN = curAssertion.GetOp1().GetVN(); ValueNum lenVN = curAssertion.GetOp2().GetVN(); - if (!curAssertion.GetOp2().IsVNNeverNegative() && (lenVN != preferredBoundVN) && - !((normalLclVN == lenVN) && pRange->LowerLimit().IsConstant() && - (pRange->LowerLimit().GetConstant() >= 0))) - { - Range boundRange = GetRangeFromAssertionsWorker(comp, lenVN, assertions, budget - 1, visited); - if (!boundRange.LowerLimit().IsConstant() || (boundRange.LowerLimit().GetConstant() < 0)) - { - continue; - } - } + assert(curAssertion.GetOp2().GetCns() == 0); + assert(curAssertion.GetOp2().IsVNNeverNegative()); if (normalLclVN == indexVN) { @@ -1644,18 +1618,13 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler* comp assert(limit.IsBinOpArray() || limit.IsConstant()); - // Symbolic limits require a non-negative base, not merely a VN used in some bounds check. - // The preferred bound is already known non-negative at the query site. Other bounds - // need independent proof: new int[n] on another path does not imply n >= 0 here. + // A checked-bound VN can be negative on paths that skip its allocation. + // Symbolic limits require a non-negative base; the preferred bound is the length being checked. if (limit.IsBinOpArray() && (limit.vn != preferredBoundVN) && !((limit.vn == curAssertion.GetOp2().GetVN()) && curAssertion.GetOp2().IsVNNeverNegative()) && !comp->vnStore->IsVNNeverNegative(limit.vn)) { - Range boundRange = GetRangeFromAssertionsWorker(comp, limit.vn, assertions, budget, visited); - if (!boundRange.LowerLimit().IsConstant() || (boundRange.LowerLimit().GetConstant() < 0)) - { - continue; - } + continue; } #ifdef DEBUG @@ -2457,7 +2426,7 @@ void Indent(int indent) // block - the block that contains `expr`; // expr - expression to compute the range for; // pRange - [Out] range of the expression; -// preferredBoundVN - a bound known non-negative at the query site, preferred over constant limits. +// preferredBoundVN - a value number of the preferred bound. // // Return Value: // false if the range is unknown or determined to overflow. diff --git a/src/coreclr/jit/rangecheck.h b/src/coreclr/jit/rangecheck.h index ce0ae9ff630569..64c38ac7b687b9 100644 --- a/src/coreclr/jit/rangecheck.h +++ b/src/coreclr/jit/rangecheck.h @@ -76,8 +76,8 @@ struct Limit { enum LimitType { - keUndef, // The limit is yet to be computed. - keBinOpArray, // A non-negative bound VN plus a constant. + keUndef, // The limit is yet to be computed. + keBinOpArray, keConstant, keDependent, // The limit is dependent on some other value. keUnknown, // The limit could not be determined. diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index e3ffd47619bda3..51ac48dbd8516a 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -7918,7 +7918,7 @@ bool ValueNumStore::IsVNCheckedBound(ValueNum vn) if (m_checkedBoundVNs.TryGetValue(vn, &dummy)) { // This VN appeared as the conservative VN of the length argument of some - // GT_BOUNDS_CHECK node. This does not imply the VN is non-negative on other paths. + // GT_BOUNDS_CHECK node. return true; } if (IsVNArrLen(vn)) diff --git a/src/tests/JIT/Regression_ro_2/Runtime_133821.cs b/src/tests/JIT/Regression_ro_2/Runtime_133821.cs index 5704872135bf23..b1586035877936 100644 --- a/src/tests/JIT/Regression_ro_2/Runtime_133821.cs +++ b/src/tests/JIT/Regression_ro_2/Runtime_133821.cs @@ -13,45 +13,14 @@ public class Runtime_133821 public static void TestEntryPoint() { int[] arr = { 10, 11, 12, 13, 14, 15, 16, 17 }; - Assert.Equal(10, Signed(arr, 4, 2, true)); - Assert.Equal(14, Signed(arr, 4, 4, false)); - Assert.Equal(-1, Signed(arr, 4, 8, false)); - Assert.Throws(() => Signed(arr, -5, -3, false)); - Assert.Throws(() => SignedOffset(arr, -5, -3, false)); - Assert.Throws(() => SignedReversed(arr, -5, -3, false)); - Assert.Throws(() => Equal(arr, -3, -3, false)); - Assert.Throws(() => Unsigned(arr, -3, -5, false)); - - Assert.Equal(0, Subtract(0, 0)); - Assert.Equal(7, Subtract(10, 3)); - Assert.Equal(int.MaxValue, Subtract(int.MaxValue, 0)); - Assert.Equal(0, Subtract(int.MaxValue, int.MaxValue)); - Assert.Equal(-1, Subtract(10, 11)); - Assert.Equal(-1, Subtract(10, -1)); - Assert.Equal(-1, Subtract(int.MaxValue, int.MinValue)); - Assert.Equal(-1, Subtract(int.MinValue, int.MaxValue)); - } - - [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] - private static int Signed(int[] arr, int n, int k, bool cond) - { - if (cond) - { - int[] tmp = new int[n]; - s_sink = tmp[0]; - } - - int i = 0; - if (k >= n) - { - i = k; - } - - return i < arr.Length ? arr[i] : -1; + Assert.Equal(10, Test(arr, 4, 2, true)); + Assert.Equal(14, Test(arr, 4, 4, false)); + Assert.Equal(-1, Test(arr, 4, 8, false)); + Assert.Throws(() => Test(arr, -5, -3, false)); } [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] - private static int SignedOffset(int[] arr, int n, int k, bool cond) + private static int Test(int[] arr, int n, int k, bool cond) { if (cond) { @@ -59,26 +28,6 @@ private static int SignedOffset(int[] arr, int n, int k, bool cond) s_sink = tmp[0]; } - int i = 0; - if (k >= n - 1) - { - i = k + 1; - } - - return i < arr.Length ? arr[i] : -1; - } - - [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] - private static int SignedReversed(int[] arr, int n, int k, bool cond) - { - if (cond) - { - int[] tmp = new int[n]; - s_sink = tmp[0]; - int[] other = new int[k]; - s_sink = other[0]; - } - int i = 0; if (k >= n) { @@ -87,52 +36,4 @@ private static int SignedReversed(int[] arr, int n, int k, bool cond) return i < arr.Length ? arr[i] : -1; } - - [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] - private static int Equal(int[] arr, int n, int k, bool cond) - { - if (cond) - { - int[] tmp = new int[n]; - s_sink = tmp[0]; - } - - int i = 0; - if (k == n) - { - i = k; - } - - return i < arr.Length ? arr[i] : -1; - } - - [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] - private static int Unsigned(int[] arr, int n, int k, bool cond) - { - if (cond) - { - int[] tmp = new int[n]; - s_sink = tmp[0]; - } - - int i = 0; - if ((uint)k < (uint)n) - { - i = k; - } - - return i < arr.Length ? arr[i] : -1; - } - - [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] - private static int Subtract(int length, int start) - { - if (length < 0 || (uint)start > (uint)length) - { - return -1; - } - - int remaining = length - start; - return remaining < 0 ? -2 : remaining; - } }