From 1b906a667000d1ca5264546799e5e8cabdee602a Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sun, 26 Apr 2026 13:57:20 -0700 Subject: [PATCH 1/2] JIT: coalesce constant-indexed bounds checks within a block Add a new phase `optBoundsCheckCoalesce` that runs before assertion prop, looking for sequences of bounds checks that can be collapsed into a single dominating check. For example: `a[0] + a[1] + a[2] + a[3]` produces four bounds checks with indices 0, 1, 2, 3 and the same length VN. The phase rewrites the first check index to 3 and marks the other three checks as "in bound" so they get removed during assertion prop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/CMakeLists.txt | 1 + src/coreclr/jit/boundscheckcoalesce.cpp | 236 ++++++++++++++++++++++++ src/coreclr/jit/compiler.cpp | 4 + src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/compphases.h | 1 + 5 files changed, 243 insertions(+) create mode 100644 src/coreclr/jit/boundscheckcoalesce.cpp diff --git a/src/coreclr/jit/CMakeLists.txt b/src/coreclr/jit/CMakeLists.txt index e7e69887486830..e5228ee28be3b4 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..98dc5a1136c40b --- /dev/null +++ b/src/coreclr/jit/boundscheckcoalesce.cpp @@ -0,0 +1,236 @@ +// 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: +// +// 1. Strengthens the FIRST bounds check in the group by replacing its +// constant index with the maximum constant index in the group. +// 2. Marks all other bounds checks in the group with GTF_CHK_INDEX_INBND +// so the existing assertion-prop COMMA handler removes them. +// +// 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 and tag the other three for removal. Forward assertion prop then +// drops them as redundant. +// +// Safety: +// * Strengthening is sound: if the new (stronger) check passes, all the +// original (weaker) checks would have passed too. If it fails, one of +// the original checks would have failed too -- both throw the same +// IndexOutOfRangeException. +// * We only coalesce bounds checks that are not separated by side effects +// (calls, indirect/heap stores, atomic ops, memory barriers, or stores +// to locals that are live in/out of an exception handler in a containing +// try region). Other bounds checks between members of the group are not +// barriers (they only throw IOOB, which is the same exception type our +// strengthened check throws). +// * We require all candidates in the group to have the same length VN +// and constant non-negative indices. The first BC's index must itself +// be a constant so it can be mutated in place. +// +// This phase runs before PHASE_ASSERTION_PROP_MAIN so that the existing +// forward direction of assertion prop sees the strengthened first BC and +// removes the marked-redundant later BCs. +// + +#include "jitpch.h" + +#ifdef _MSC_VER +#pragma hdrstop +#endif + +namespace +{ +struct BoundsCheckCandidate +{ + GenTreeBoundsChk* m_bc; + Statement* m_stmt; + ValueNum m_lenVN; + int m_offset; + int m_barrierCount; + + BoundsCheckCandidate(GenTreeBoundsChk* bc, Statement* stmt, ValueNum lenVN, int offset, int barrierCount) + : m_bc(bc) + , m_stmt(stmt) + , m_lenVN(lenVN) + , m_offset(offset) + , m_barrierCount(barrierCount) + { + } +}; + +//------------------------------------------------------------------------ +// IsSideEffectBarrier: check if a node blocks bounds check coalescing +// +// Returns true if a node may have a side effect that should prevent us from +// reordering an earlier bounds-check failure across it. +// +// Stores to tracked locals that are not live in/out of any exception handler +// are not barriers: they cannot be observed if a bounds-check failure is +// reordered to before them. +// +bool IsSideEffectBarrier(Compiler* comp, GenTree* node, bool blockIsInsideTry) +{ + if (node->IsCall()) + { + return true; + } + if (node->OperIs(GT_MEMORYBARRIER)) + { + return true; + } + if (node->OperIsAtomicOp()) + { + return true; + } + if (node->OperIsStore()) + { + if (!node->OperIsLocalStore()) + { + return true; + } + if (!blockIsInsideTry) + { + 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; + } + + bool modified = false; + CompAllocator alloc(getAllocator(CMK_AssertionProp)); + + // Per-block scratch state, reused across blocks. The candidates stack + // holds the "head" (first) candidate in each (barrierCount, lenVN) group; + // followers are tagged GTF_CHK_INDEX_INBND immediately and not retained. + // groupMap maps a packed (barrierCount, lenVN) key to the candidate index + // of the group head. + typedef JitHashTable, int> GroupMap; + ArrayStack candidates(alloc); + GroupMap groupMap(alloc); + + auto const makeKey = [](int barrierCount, ValueNum lenVN) -> UINT64 { + return (static_cast(static_cast(barrierCount)) << 32) | static_cast(lenVN); + }; + + for (BasicBlock* const block : Blocks()) + { + candidates.Reset(); + groupMap.RemoveAll(); + int barrierCount = 0; + bool const blockIsInsideTry = block->hasTryIndex(); + + for (Statement* const stmt : block->Statements()) + { + for (GenTree* const node : stmt->TreeList()) + { + if (IsSideEffectBarrier(this, node, blockIsInsideTry)) + { + barrierCount++; + continue; + } + + if (!node->OperIs(GT_BOUNDS_CHECK)) + { + continue; + } + + 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; + } + + ValueNum const lenVN = vnStore->VNConservativeNormalValue(bc->GetArrayLength()->gtVNPair); + if (lenVN == ValueNumStore::NoVN) + { + continue; + } + + UINT64 const key = makeKey(barrierCount, lenVN); + int headIndex; + if (!groupMap.Lookup(key, &headIndex)) + { + // First member of this group: record it as the head and keep it + // in the candidates stack so we can strengthen it later. + groupMap.Set(key, candidates.Height()); + candidates.Emplace(bc, stmt, lenVN, offset, barrierCount); + continue; + } + + // Follower: tag for forward assertion prop to splice out, and + // bump the head's running max offset. + BoundsCheckCandidate& head = candidates.BottomRef(headIndex); + JITDUMP("BC coalesce in " FMT_BB ": marking [%06u] (offset %d) as redundant of [%06u]\n", block->bbNum, + dspTreeID(bc), offset, dspTreeID(head.m_bc)); + bc->gtFlags |= GTF_CHK_INDEX_INBND; + if (offset > head.m_offset) + { + head.m_offset = offset; + } + } + } + + // Strengthen each group head whose recorded max exceeds its original + // index. Heads with no stronger follower are left alone -- existing + // forward assertion prop already handles equal-or-weaker followers. + 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 a7e591163e109c..499fef17c5ee23 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 c5341a95bfc0e8..9d1cc14655bb31 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7281,6 +7281,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) From 8ef7a3cf5023aa2385341b296143abab5d9e0ced Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 27 Apr 2026 11:25:57 -0700 Subject: [PATCH 2/2] Address PR feedback: simplify barrier rule and drop redundant tagging - Use OperMayThrow + GTF_ORDER_SIDEEFF as the side-effect barrier rule instead of an ad-hoc list (memorybarrier/atomic). GT_BOUNDS_CHECK is exempted since IOOB is the same exception class our strengthened check throws. - Switch from block->hasTryIndex() to block->HasPotentialEHSuccs(this) for the EH-reachability test, matching usage elsewhere in the JIT. - Drop GTF_CHK_INDEX_INBND tagging on followers; strengthening only the head is enough -- forward assertion prop drops the followers. Add regression tests covering exception-ordering invariants: divide/NRE between BCs, IOOB on short arrays, and locals live into catch/finally handlers in a try block. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/boundscheckcoalesce.cpp | 63 +++++----- .../JIT/opt/RangeChecks/ElidedBoundsChecks.cs | 108 ++++++++++++++++++ 2 files changed, 142 insertions(+), 29 deletions(-) diff --git a/src/coreclr/jit/boundscheckcoalesce.cpp b/src/coreclr/jit/boundscheckcoalesce.cpp index 98dc5a1136c40b..eae938c632dd4c 100644 --- a/src/coreclr/jit/boundscheckcoalesce.cpp +++ b/src/coreclr/jit/boundscheckcoalesce.cpp @@ -6,17 +6,14 @@ // // 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: -// -// 1. Strengthens the FIRST bounds check in the group by replacing its -// constant index with the maximum constant index in the group. -// 2. Marks all other bounds checks in the group with GTF_CHK_INDEX_INBND -// so the existing assertion-prop COMMA handler removes them. +// 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 and tag the other three for removal. Forward assertion prop then -// drops them as redundant. +// to 3; forward assertion prop then drops the other three as redundant. // // Safety: // * Strengthening is sound: if the new (stronger) check passes, all the @@ -24,18 +21,20 @@ // the original checks would have failed too -- both throw the same // IndexOutOfRangeException. // * We only coalesce bounds checks that are not separated by side effects -// (calls, indirect/heap stores, atomic ops, memory barriers, or stores -// to locals that are live in/out of an exception handler in a containing -// try region). Other bounds checks between members of the group are not -// barriers (they only throw IOOB, which is the same exception type our -// strengthened check throws). +// that could change observable exception ordering: calls, any other +// potentially-throwing node (div/mod, checked arithmetic, faulting +// indirections / null checks, etc.), `GTF_ORDER_SIDEEFF` (e.g. volatile +// loads), heap-visible stores, and stores to locals that are live across +// an exception handler reachable from this block. Other bounds checks +// between members of the group are not barriers (they only throw IOOB, +// the same exception type our strengthened check throws). // * We require all candidates in the group to have the same length VN // and constant non-negative indices. The first BC's index must itself // be a constant so it can be mutated in place. // // This phase runs before PHASE_ASSERTION_PROP_MAIN so that the existing // forward direction of assertion prop sees the strengthened first BC and -// removes the marked-redundant later BCs. +// drops the redundant followers. // #include "jitpch.h" @@ -70,21 +69,28 @@ struct BoundsCheckCandidate // Returns true if a node may have a side effect that should prevent us from // reordering an earlier bounds-check failure across it. // -// Stores to tracked locals that are not live in/out of any exception handler -// are not barriers: they cannot be observed if a bounds-check failure is -// reordered to before them. +// Bounds checks themselves are not barriers: their only exception is IOOB, +// the same exception type our strengthened check throws. +// +// Stores to tracked locals that are not live across any exception handler +// reachable from this block are not barriers: they cannot be observed if a +// bounds-check failure is reordered to before them. // -bool IsSideEffectBarrier(Compiler* comp, GenTree* node, bool blockIsInsideTry) +bool IsSideEffectBarrier(Compiler* comp, GenTree* node, bool blockHasEHSuccs) { if (node->IsCall()) { return true; } - if (node->OperIs(GT_MEMORYBARRIER)) + if (node->OperIs(GT_BOUNDS_CHECK)) + { + return false; + } + if (node->OperMayThrow(comp)) { return true; } - if (node->OperIsAtomicOp()) + if ((node->gtFlags & GTF_ORDER_SIDEEFF) != 0) { return true; } @@ -94,7 +100,7 @@ bool IsSideEffectBarrier(Compiler* comp, GenTree* node, bool blockIsInsideTry) { return true; } - if (!blockIsInsideTry) + if (!blockHasEHSuccs) { return false; } @@ -129,7 +135,7 @@ PhaseStatus Compiler::optBoundsCheckCoalesce() // Per-block scratch state, reused across blocks. The candidates stack // holds the "head" (first) candidate in each (barrierCount, lenVN) group; - // followers are tagged GTF_CHK_INDEX_INBND immediately and not retained. + // followers only update the head's running max offset and are not retained. // groupMap maps a packed (barrierCount, lenVN) key to the candidate index // of the group head. typedef JitHashTable, int> GroupMap; @@ -144,14 +150,14 @@ PhaseStatus Compiler::optBoundsCheckCoalesce() { candidates.Reset(); groupMap.RemoveAll(); - int barrierCount = 0; - bool const blockIsInsideTry = block->hasTryIndex(); + int barrierCount = 0; + bool const blockHasEHSuccs = block->HasPotentialEHSuccs(this); for (Statement* const stmt : block->Statements()) { for (GenTree* const node : stmt->TreeList()) { - if (IsSideEffectBarrier(this, node, blockIsInsideTry)) + if (IsSideEffectBarrier(this, node, blockHasEHSuccs)) { barrierCount++; continue; @@ -197,12 +203,11 @@ PhaseStatus Compiler::optBoundsCheckCoalesce() continue; } - // Follower: tag for forward assertion prop to splice out, and - // bump the head's running max offset. + // Follower: bump the head's running max offset. Once we + // strengthen the head, forward assertion prop will drop us. BoundsCheckCandidate& head = candidates.BottomRef(headIndex); - JITDUMP("BC coalesce in " FMT_BB ": marking [%06u] (offset %d) as redundant of [%06u]\n", block->bbNum, + JITDUMP("BC coalesce in " FMT_BB ": [%06u] (offset %d) is redundant given [%06u]\n", block->bbNum, dspTreeID(bc), offset, dspTreeID(head.m_bc)); - bc->gtFlags |= GTF_CHK_INDEX_INBND; if (offset > head.m_offset) { head.m_offset = offset; diff --git a/src/tests/JIT/opt/RangeChecks/ElidedBoundsChecks.cs b/src/tests/JIT/opt/RangeChecks/ElidedBoundsChecks.cs index d6196e0a1922a9..7d01a8fbfab518 100644 --- a/src/tests/JIT/opt/RangeChecks/ElidedBoundsChecks.cs +++ b/src/tests/JIT/opt/RangeChecks/ElidedBoundsChecks.cs @@ -95,6 +95,74 @@ 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; + } + } + [Fact] public static int TestEntryPoint() { @@ -139,6 +207,46 @@ 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; + return 100; } }