JIT: unpin locals whose value is provably non-movable#129110
Draft
AndyAyersMS wants to merge 2 commits into
Draft
JIT: unpin locals whose value is provably non-movable#129110AndyAyersMS wants to merge 2 commits into
AndyAyersMS wants to merge 2 commits into
Conversation
Treat GTF_ICON_STATIC_HDL constants as no-gc defs, and add a new PHASE_UNPIN_LOCALS phase that runs after local morph and solves a small monotone forward dataflow tracking whether all defs of each local are no-gc (direct or via a LCL_VAR with the same property). Any pinned local whose value is provably non-movable is then unpinned, and the redundant pin/unpin stores get DCE'd. The phase is a no-op for MinOpts and bails early when the method has no pinned locals. Catches chains like RVA-static or stackalloc threaded through a Span<>._reference temp; the existing lvaMarkLclRefs path continues to catch chains that only collapse to a direct no-gc value after morph constant propagation. Fixes dotnet#13190. Fixes dotnet#118802. Fixes dotnet#127691. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
area-CodeGen-coreclr
Member
Author
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Member
Author
|
Also wondering if we should be retyping these locals, though that leads back to the land of mixed-type comparisons and such... |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new CoreCLR JIT optimization pass to clear lvPinned on locals whose definitions are proven to be GC-immovable (e.g., stack addresses, frozen handles, static/RVA handles), enabling later DCE to remove redundant pin/unpin stores and associated overhead.
Changes:
- Extend
GenTree::IsNotGcDef()to treatGTF_ICON_STATIC_HDL(andSTATIC_HDL + offset) as non-GC / non-movable definitions. - Add a new JIT phase (
PHASE_UNPIN_LOCALS) after local morph that computes a per-local “all defs are no-gc” property (with simple copy propagation) and unpins eligible locals. - Add new JIT regression test projects covering fixed-over-stackalloc and fixed-over-RVA-static scenarios.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/gentree.h | Classifies static handle constants (and handle+offset) as IsNotGcDef() to enable “no-gc” reasoning. |
| src/coreclr/jit/lclmorph.cpp | Implements fgUnpinNonMovableLocals() monotone analysis and clears lvPinned for proven non-movable locals. |
| src/coreclr/jit/compphases.h | Introduces PHASE_UNPIN_LOCALS in the phase list. |
| src/coreclr/jit/compiler.h | Declares the new phase entry point. |
| src/coreclr/jit/compiler.cpp | Wires PHASE_UNPIN_LOCALS into the compilation pipeline after local morph. |
| src/tests/JIT/opt/DSE/Regressions/PinnedStackAllocDoesNotEmitPinStore.csproj | Adds a new JIT regression test project for stackalloc + fixed patterns. |
| src/tests/JIT/opt/DSE/Regressions/PinnedStackAllocDoesNotEmitPinStore.cs | Adds functional tests that exercise stackalloc + fixed usage patterns. |
| src/tests/JIT/opt/DSE/Regressions/PinnedRvaStaticDoesNotEmitPinStore.csproj | Adds a new JIT regression test project for RVA-static span patterns. |
| src/tests/JIT/opt/DSE/Regressions/PinnedRvaStaticDoesNotEmitPinStore.cs | Adds functional tests that exercise RVA-static span + fixed usage patterns. |
Comment on lines
+2502
to
+2506
| JITDUMP("fgUnpinNonMovableLocals: %u local%s unpinned after %u iteration%s\n", unpinned, unpinned == 1 ? "" : "s", | ||
| iterations, iterations == 1 ? "" : "s"); | ||
|
|
||
| return PhaseStatus::MODIFIED_NOTHING; | ||
| } |
Comment on lines
+60
to
+76
| [Fact] | ||
| public static void ReadFirst_ReturnsZero() | ||
| { | ||
| Assert.Equal(0, ReadFirst()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void Sum_ReturnsExpected() | ||
| { | ||
| Assert.Equal(1 + 2 + 3 + 4 + 5 + 6 + 7 + 8, Sum()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public static void SumViaTwoFixed_ReturnsExpected() | ||
| { | ||
| Assert.Equal(1 + 8, SumViaTwoFixed()); | ||
| } |
Member
For that specific one I have #119674 PR which is basically ready, need to clarify NAOT behavior. I am not sure yours is correct in assumption? E.g for unloadable ALC context where pinning effectively keeps it alive |
This was referenced Jun 8, 2026
Member
Author
I will leave that part for your PR. The dataflow analysis provides the bulk of the benefit here. |
Remove the GTF_ICON_STATIC_HDL arm of IsNotGcDef. As @EgorBo noted on dotnet#129110, the static-data address can live in a collectible ALC's loader heap, where pinning may be what's keeping the LoaderAllocator reachable. The safe answer is the canOmitPinning JIT-EE API in dotnet#119674, which asks the VM to verify !LoaderAllocator->CanUnload(). The stackalloc fix in PHASE_UNPIN_LOCALS is unaffected: it still picks up the GT_LCL_ADDR path that was already in IsNotGcDef. SPMI asmdiffs loses only ~3K bytes out of -190K from this revert. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Treat GTF_ICON_STATIC_HDL constants as no-gc defs, and add a new PHASE_UNPIN_LOCALS phase that runs after local morph and solves a small monotone forward dataflow tracking whether all defs of each local are no-gc (direct or via a LCL_VAR with the same property). Any pinned local whose value is provably non-movable is then unpinned, and the redundant pin/unpin stores get DCE'd.
The phase is a no-op for MinOpts and bails early when the method has no pinned locals. Catches chains like RVA-static or stackalloc threaded through a Span<>._reference temp; the existing lvaMarkLclRefs path continues to catch chains that only collapse to a direct no-gc value after morph constant propagation.
Fixes #13190.
Fixes #118802.
Fixes #127691.