diff --git a/src/coreclr/jit/CMakeLists.txt b/src/coreclr/jit/CMakeLists.txt index dd83c6891518fe..f5ede6d5d9f89c 100644 --- a/src/coreclr/jit/CMakeLists.txt +++ b/src/coreclr/jit/CMakeLists.txt @@ -97,6 +97,7 @@ set( JIT_SOURCES asyncanalysis.cpp bitset.cpp block.cpp + boundscheckcoalesce.cpp buildstring.cpp codegencommon.cpp codegenlinear.cpp diff --git a/src/coreclr/jit/boundscheckcoalesce.cpp b/src/coreclr/jit/boundscheckcoalesce.cpp new file mode 100644 index 00000000000000..3615a96bb8ee15 --- /dev/null +++ b/src/coreclr/jit/boundscheckcoalesce.cpp @@ -0,0 +1,253 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// +// Bounds Check Coalescing +// +// Within a single block, when multiple GT_BOUNDS_CHECK nodes share the same +// length VN and use constant indices, only the bounds check with the largest +// constant index is actually needed. This pass finds such groups and +// strengthens the first bounds check in the group by replacing its constant +// index with the maximum constant index in the group. Forward assertion prop +// then drops the now-redundant later bounds checks. +// +// Example: `a[0] + a[1] + a[2] + a[3]` produces four bounds checks with +// indices 0, 1, 2, 3 and the same length. We rewrite the first BC's index +// to 3; forward assertion prop then drops the other three as redundant. +// +// We ensure no observable side effects (other than a bounds check exception +// can occur between the original check and the subsequent checks. +// +// This phase runs before assertion prop, which then optimizes away +// the trailing, now-redundant checks. +// + +#include "jitpch.h" + +#ifdef _MSC_VER +#pragma hdrstop +#endif + +namespace +{ +struct BoundsCheckCandidate +{ + // leading bounds check in a candidate set + GenTreeBoundsChk* m_bc; + + // array length being checked + ValueNum m_lenVN; + + // Max index being checked + int m_offset; + + BoundsCheckCandidate(GenTreeBoundsChk* bc, ValueNum lenVN, int offset) + : m_bc(bc) + , m_lenVN(lenVN) + , m_offset(offset) + { + } +}; + +//------------------------------------------------------------------------ +// IsSideEffectBarrier: check if a node blocks bounds check coalescing +// +// Arguments: +// comp - the compiler instance +// node - the node to check +// blockHasEHSuccs - whether the block containing the node has reachable EH successors +// +// Returns: +// true if a node may have a side effect that should prevent us from +// coalescing bounds checks across it. Uses the per-node +// (non-summary) effect flags from GenTree::OperEffects. +// +bool IsSideEffectBarrier(Compiler* comp, GenTree* node, bool blockHasEHSuccs) +{ + ExceptionSetFlags exSet; + GenTreeFlags const effects = node->OperEffects(comp, &exSet); + + if ((effects & GTF_CALL) != 0) + { + return true; + } + + if ((effects & GTF_EXCEPT) != 0) + { + if ((exSet & ~ExceptionSetFlags::IndexOutOfRangeException) != ExceptionSetFlags::None) + { + return true; + } + } + + if ((effects & GTF_ASG) != 0) + { + if (!node->OperIsLocalStore()) + { + return true; + } + if (!blockHasEHSuccs) + { + return false; + } + LclVarDsc const* const dsc = comp->lvaGetDesc(node->AsLclVarCommon()); + return !dsc->lvTracked || dsc->lvLiveInOutOfHndlr; + } + + return false; +} +} // namespace + +//------------------------------------------------------------------------ +// optBoundsCheckCoalesce: Coalesce bounds checks within each block. +// +// Returns: +// Suitable phase status. +// +PhaseStatus Compiler::optBoundsCheckCoalesce() +{ + if (!doesMethodHaveBoundsChecks()) + { + JITDUMP("Method has no bounds checks\n"); + return PhaseStatus::MODIFIED_NOTHING; + } + + if (fgSsaPassesCompleted == 0) + { + return PhaseStatus::MODIFIED_NOTHING; + } + + // Track the current maximum offset seen for a given length VN + // optimization barrier count. + // + struct Key + { + int m_barrierCount; + ValueNum m_lengthVN; + + Key(int barrierCount, ValueNum lengthVN) + : m_barrierCount(barrierCount) + , m_lengthVN(lengthVN) + { + } + + bool operator==(const Key& other) const + { + return (m_barrierCount == other.m_barrierCount) && (m_lengthVN == other.m_lengthVN); + } + + static bool Equals(const Key& x, const Key& y) + { + return (x.m_barrierCount == y.m_barrierCount) && (x.m_lengthVN == y.m_lengthVN); + } + + static unsigned GetHashCode(const Key& x) + { + return (unsigned)x.m_lengthVN ^ (unsigned)x.m_barrierCount; + } + }; + + typedef JitHashTable GroupMap; + + bool modified = false; + CompAllocator alloc(getAllocator(CMK_AssertionProp)); + + ArrayStack candidates(alloc); + GroupMap groupMap(alloc); + + for (BasicBlock* const block : Blocks()) + { + candidates.Reset(); + groupMap.RemoveAll(); + int barrierCount = 0; + bool const blockHasEHSuccs = block->HasPotentialEHSuccs(this); + + for (Statement* const stmt : block->Statements()) + { + for (GenTree* const node : stmt->TreeList()) + { + if (node->OperIs(GT_BOUNDS_CHECK)) + { + GenTreeBoundsChk* const bc = node->AsBoundsChk(); + if (bc->gtThrowKind != SCK_RNGCHK_FAIL) + { + continue; + } + + GenTree* const idx = bc->GetIndex(); + if (!idx->IsIntCnsFitsInI32()) + { + continue; + } + + int const offset = static_cast(idx->AsIntCon()->IconValue()); + if (offset < 0) + { + continue; + } + + // Look through comma-wrapped length nodes. + // + GenTree* const lenNode = bc->GetArrayLength()->gtEffectiveVal(); + ValueNum const lenVN = vnStore->VNConservativeNormalValue(lenNode->gtVNPair); + if (lenVN == ValueNumStore::NoVN) + { + continue; + } + + Key key(barrierCount, lenVN); + int headIndex; + if (!groupMap.Lookup(key, &headIndex)) + { + // First constant-index bounds check with this length VN and this barrier count. + // Start a new group. + // + groupMap.Set(key, candidates.Height()); + candidates.Emplace(bc, lenVN, offset); + continue; + } + + // Following bounds check. See if this is a new max index. + // + BoundsCheckCandidate& head = candidates.BottomRef(headIndex); + JITDUMP("BC coalesce in " FMT_BB ": [%06u] (offset %d) is redundant given [%06u]\n", block->bbNum, + dspTreeID(bc), offset, dspTreeID(head.m_bc)); + if (offset > head.m_offset) + { + head.m_offset = offset; + } + continue; + } + + if (IsSideEffectBarrier(this, node, blockHasEHSuccs)) + { + barrierCount++; + continue; + } + } + } + + // Revise the check made by the first entry in each group, if we + // found a subsequent check at a higher constant index. + // + for (int i = 0; i < candidates.Height(); i++) + { + BoundsCheckCandidate& head = candidates.BottomRef(i); + GenTreeIntCon* const idxCns = head.m_bc->GetIndex()->AsIntCon(); + int const original = static_cast(idxCns->IconValue()); + if (head.m_offset == original) + { + continue; + } + + JITDUMP("BC coalesce in " FMT_BB ": strengthen [%06u] offset %d -> %d (lenVN " FMT_VN ")\n", block->bbNum, + dspTreeID(head.m_bc), original, head.m_offset, head.m_lenVN); + + idxCns->SetIconValue(head.m_offset); + idxCns->gtVNPair.SetBoth(vnStore->VNForIntCon(head.m_offset)); + modified = true; + } + } + + return modified ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; +} diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 24b980ccc8f755..dcbe929baae14c 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4807,6 +4807,10 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl if (doAssertionProp) { + // Coalesce groups of constant-indexed bounds checks. + // + DoPhase(this, PHASE_BOUNDS_CHECK_COALESCE, &Compiler::optBoundsCheckCoalesce); + // Assertion propagation // DoPhase(this, PHASE_ASSERTION_PROP_MAIN, &Compiler::optAssertionPropMain); diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 6d83b3ff0030e5..52f8c5aac917f6 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7500,6 +7500,7 @@ class Compiler PhaseStatus optCloneLoops(); PhaseStatus optRangeCheckCloning(); + PhaseStatus optBoundsCheckCoalesce(); void optCloneLoop(FlowGraphNaturalLoop* loop, LoopCloneContext* context); PhaseStatus optUnrollLoops(); // Unrolls loops (needs to have cost info) bool optTryUnrollLoop(FlowGraphNaturalLoop* loop, bool* changedIR); diff --git a/src/coreclr/jit/compphases.h b/src/coreclr/jit/compphases.h index a40ecfd7fe932b..8cc24fd02239bc 100644 --- a/src/coreclr/jit/compphases.h +++ b/src/coreclr/jit/compphases.h @@ -102,6 +102,7 @@ CompPhaseNameMacro(PHASE_OPTIMIZE_VALNUM_CSES, "Optimize Valnum CSEs", CompPhaseNameMacro(PHASE_VN_COPY_PROP, "VN based copy prop", false, -1, false) CompPhaseNameMacro(PHASE_VN_BASED_INTRINSIC_EXPAND, "VN based intrinsic expansion", false, -1, false) CompPhaseNameMacro(PHASE_OPTIMIZE_BRANCHES, "Redundant branch opts", false, -1, false) +CompPhaseNameMacro(PHASE_BOUNDS_CHECK_COALESCE, "Coalesce bounds checks", false, -1, false) CompPhaseNameMacro(PHASE_ASSERTION_PROP_MAIN, "Assertion prop", false, -1, false) CompPhaseNameMacro(PHASE_RANGE_CHECK_CLONING, "Clone blocks with range checks", false, -1, false) CompPhaseNameMacro(PHASE_IF_CONVERSION, "If conversion", false, -1, false) diff --git a/src/tests/JIT/opt/RangeChecks/ElidedBoundsChecks.cs b/src/tests/JIT/opt/RangeChecks/ElidedBoundsChecks.cs index d6196e0a1922a9..565f311657b726 100644 --- a/src/tests/JIT/opt/RangeChecks/ElidedBoundsChecks.cs +++ b/src/tests/JIT/opt/RangeChecks/ElidedBoundsChecks.cs @@ -95,6 +95,115 @@ static bool TryStripFirstChar(ref ReadOnlySpan span, char value) return false; } + [MethodImpl(MethodImplOptions.NoInlining)] + static int Sum4Increasing(int[] a) => a[0] + a[1] + a[2] + a[3]; + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Sum4Span(ReadOnlySpan s) => s[0] + s[1] + s[2] + s[3]; + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Sum4MixedOrder(int[] a) => a[2] + a[3] + a[0] + a[1]; + + [MethodImpl(MethodImplOptions.NoInlining)] + static int DivBetweenBCs(int[] a, int divisor) + { + // The divide must not be reordered with a[5]: when divisor == 0 we + // must observe DivideByZeroException, not IndexOutOfRangeException. + int x = a[3]; + int y = 100 / divisor; + return x + y + a[5]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int NreBetweenBCs(int[] a, int[] b) + { + // First touch of b may throw NRE; that must not be reordered with + // a[5]: when b == null we must observe NullReferenceException. + int x = a[3]; + int y = b.Length; + return x + y + a[5]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int LocalLiveInCatch(int[] a) + { + // The store `x = 99` is between two BCs in a try block whose local is + // live in the catch. It must act as a barrier: if a[3]'s BC were + // strengthened to length 6, the IOOB would fire before x=99 and the + // catch would observe x == -1 instead of 99. + int x = -1; + try + { + int t = a[3]; + x = 99; + return t + a[5]; + } + catch (IndexOutOfRangeException) + { + return x; + } + } + + static int s_finallyObserved; + + [MethodImpl(MethodImplOptions.NoInlining)] + static int LocalLiveInFinally(int[] a) + { + // Same idea, but the local is live into a finally rather than a catch. + int x = -1; + try + { + int t = a[3]; + x = 99; + return t + a[5]; + } + finally + { + s_finallyObserved = x; + } + } + + static int s_storeObserved; + + [MethodImpl(MethodImplOptions.NoInlining)] + static int StoreBetweenBCs(int[] a) + { + // A heap-visible store between two BCs must act as a barrier: if a + // is too short for a[5], the store to s_storeObserved must still be + // observable (i.e., the strengthened check must not throw IOOB + // before the store). + int t = a[3]; + s_storeObserved = 99; + return t + a[5]; + } + + static int s_callObserved; + + [MethodImpl(MethodImplOptions.NoInlining)] + static void MarkCalled() + { + s_callObserved = 99; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int CallBetweenBCs(int[] a) + { + // A call between two BCs must act as a barrier even if the callee + // doesn't throw: the call's side effects (here, writing s_callObserved) + // must remain observable when the second BC fails. + int t = a[3]; + MarkCalled(); + return t + a[5]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int TwoArrays(int[] a, int[] b) + { + // Two arrays with distinct length VNs interleaved: bounds checks + // for `a` and `b` must not coalesce into a single group. + return a[0] + b[0] + a[3] + b[1]; + } + [Fact] public static int TestEntryPoint() { @@ -139,6 +248,81 @@ public static int TestEntryPoint() if (TryStripFirstChar(ref chars, 'h') != false) return 0; + // Bounds-check coalescing: 4 constant indices, same length VN. + int[] arr4 = new int[] { 10, 20, 30, 40 }; + if (Sum4Increasing(arr4) != 100) + return 0; + if (Sum4Span(arr4) != 100) + return 0; + if (Sum4MixedOrder(arr4) != 100) + return 0; + + // Short array: must throw IndexOutOfRangeException. + Assert.Throws(() => Sum4Increasing(new int[3])); + Assert.Throws(() => Sum4MixedOrder(new int[3])); + + // Exception ordering must be preserved across non-IOOB throwers. + int[] arr6 = new int[] { 1, 2, 3, 4, 5, 6 }; + if (DivBetweenBCs(arr6, 5) != (arr6[3] + 100 / 5 + arr6[5])) + return 0; + + // divisor == 0 with a too short for a[5]: must be DivideByZero, not IOOB. + Assert.Throws(() => DivBetweenBCs(new int[4], 0)); + + // b == null with a too short for a[5]: must be NRE, not IOOB. + Assert.Throws(() => NreBetweenBCs(new int[4], null)); + + // Local live in catch handler: a[3]'s BC must not be strengthened to + // a[5] across the `x = 99` store, otherwise the catch would see -1. + if (LocalLiveInCatch(new int[4]) != 99) + return 0; + + // Local live in finally: same constraint, observed via static field. + s_finallyObserved = 0; + try + { + LocalLiveInFinally(new int[4]); + return 0; + } + catch (IndexOutOfRangeException) { } + if (s_finallyObserved != 99) + return 0; + + // Heap-visible store between BCs must remain observable when the + // second BC fails. If we (incorrectly) strengthened a[3] to a[5], + // the IOOB would fire before the store and s_storeObserved would + // stay 0. + s_storeObserved = 0; + if (StoreBetweenBCs(arr6) != (arr6[3] + arr6[5])) + return 0; + if (s_storeObserved != 99) + return 0; + s_storeObserved = 0; + Assert.Throws(() => StoreBetweenBCs(new int[4])); + if (s_storeObserved != 99) + return 0; + + // Non-throwing call between BCs must also act as a barrier. + s_callObserved = 0; + if (CallBetweenBCs(arr6) != (arr6[3] + arr6[5])) + return 0; + if (s_callObserved != 99) + return 0; + s_callObserved = 0; + Assert.Throws(() => CallBetweenBCs(new int[4])); + if (s_callObserved != 99) + return 0; + + // Distinct length VNs must not be merged into a single group. + int[] a4 = new int[] { 1, 2, 3, 4 }; + int[] b2 = new int[] { 10, 20 }; + if (TwoArrays(a4, b2) != (a4[0] + b2[0] + a4[3] + b2[1])) + return 0; + // a too short for a[3]: must throw IOOB. + Assert.Throws(() => TwoArrays(new int[3], b2)); + // b too short for b[1]: must throw IOOB. + Assert.Throws(() => TwoArrays(a4, new int[1])); + return 100; } } diff --git a/src/tests/JIT/opt/RangeChecks/ReturnBlockRangeCheckCloning.cs b/src/tests/JIT/opt/RangeChecks/ReturnBlockRangeCheckCloning.cs index cc149ec70ffdef..fbc94ed7a04657 100644 --- a/src/tests/JIT/opt/RangeChecks/ReturnBlockRangeCheckCloning.cs +++ b/src/tests/JIT/opt/RangeChecks/ReturnBlockRangeCheckCloning.cs @@ -3,6 +3,7 @@ using System; using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; using Xunit; // Tests that consecutive range checks in return blocks are combined @@ -28,6 +29,20 @@ static int SpanAccessReturn(ReadOnlySpan span) return span[0] + span[1] + span[2] + span[3]; } + // The element loads dereference span._reference, which can throw NRE. + // Coalescing the four bounds checks by strengthening the first one to + // index 3 would change the observable exception from NRE (thrown by the + // first element load when the bounds check at index 0 passes) into IOOB + // (thrown by the strengthened check at index 3 when _length is smaller). + // This method exists so the test below can verify the precedence is + // preserved: a stack-resident Span with null _reference and _length = 2 + // must throw NullReferenceException, not IndexOutOfRangeException. + [MethodImpl(MethodImplOptions.NoInlining)] + static int NullRefSpanAccessReturn(ReadOnlySpan span) + { + return span[0] + span[1] + span[2] + span[3]; + } + [Fact] public static int TestEntryPoint() { @@ -70,6 +85,27 @@ public static int TestEntryPoint() if (!threwOffset) return 0; + // A ReadOnlySpan with null _reference and _length = 2. + // Accessing span[0] passes the bounds check, then the element load + // dereferences null and must throw NullReferenceException. The bounds + // check for span[2] would throw IndexOutOfRangeException, but we must + // never see that here -- the NRE on span[0]'s element load comes first. + // If a future change to bounds-check coalescing unsoundly strengthens + // the first check to index 3 and removes the followers, IOOB would + // surface in place of NRE and this test would fail. + ReadOnlySpan nullRefSpan = MemoryMarshal.CreateReadOnlySpan(ref Unsafe.NullRef(), 2); + bool threwNullRef = false; + try + { + NullRefSpanAccessReturn(nullRefSpan); + } + catch (NullReferenceException) + { + threwNullRef = true; + } + if (!threwNullRef) + return 0; + return 100; } } diff --git a/src/tests/JIT/opt/RangeChecks/ReturnBlockRangeCheckCloning.csproj b/src/tests/JIT/opt/RangeChecks/ReturnBlockRangeCheckCloning.csproj index de6d5e08882e86..b87c12509ab4f3 100644 --- a/src/tests/JIT/opt/RangeChecks/ReturnBlockRangeCheckCloning.csproj +++ b/src/tests/JIT/opt/RangeChecks/ReturnBlockRangeCheckCloning.csproj @@ -1,8 +1,15 @@ + + + true + True + +