JIT: Recover CSE of Cast-away-GC temps across same GcHeap epoch

[tannergooding]

Follow-up to #131625, which fixed a correctness bug where morph's "Cast away GC" temps received the same VN regardless of intervening GC safepoints, allowing CSE to reuse a stale raw address after a compacting GC moved the object.

The fix in #131625 was conservative -- VNForExpr gives every such store its own unique VN, preventing all CSE. This recovers the cases that are safe to CSE.

What this does

Introduces a dedicated fgCurGcEpochVN field (distinct from fgCurMemoryVN[GcHeap]) and uses it as the epoch in VNF_GCRefToPtrWithEpoch(byrefVN, fgCurGcEpochVN).

fgCurMemoryVN[GcHeap] advances only when the heap is mutated (fgMutateGcHeap). Allocators (CORINFO_HELP_NEWSFAST etc.) have MutatesHeap = false -- they create new objects, not modify existing ones -- so fgCurMemoryVN[GcHeap] does not advance at an allocation. But allocators are not IsNoGC, meaning the GC can compact the heap during the call. This is a soundness gap for the epoch mechanism: two fixed regions separated only by new T[n] would share the same epoch VN, making CSE of the raw-address snapshot incorrect after a compacting GC.

fgCurGcEpochVN is initialized to a fresh unique VN at each basic block entry (conservative for cross-block: no cross-block CSE of cast-away-GC temps) and is advanced at every call where !IsNoGC. The only calls that don't advance it are the pure arithmetic helpers declared isNoGC (CORINFO_HELP_LLSH, CORINFO_HELP_LRSH, CORINFO_HELP_LRSZ), for which the GC genuinely cannot run. This correctly handles the allocation case:

fixed (T* x = arr) { }   // epoch E0
T[] dst = new T[5];      // NEWSFAST: !IsNoGC → epoch advances to E1
fixed (T* x = arr) { }   // epoch E1 → different VN, no CSE (correct)

SPMI asmdiffs (6 collections, 2.78M contexts, base = #131625)

	Methods	Bytes
Improvements	154	-1,196
Regressions	7	+28

The 7 regressions are each +4 bytes in cold blocks (bbWeight=0), all cases where two stores are separated by a call that advances the epoch but doesn't actually collect. Conservatively correct; PerfScore delta is 0 for all of them.

Net PerfScore: -831 in libraries_tests alone.

---

Out of scope: fgValueNumberByrefExposedLoad already passes fgCurMemoryVN[ByrefExposed] to VNF_ByrefExposedLoad -- loads are already epoch-keyed by construction. fgValueNumberCastTree never sees a GC-to-non-GC cast because morph converts those to stores before VN runs.

Note

This PR description was drafted with GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[tannergooding]

CC. @jakobbotsch, this is what should help with the regressions and reuses the existing GC memory VN that a few other places, like VNF_ByrefExposedLoad are using.

I'm not super happy with the new VNF name, but wasn't able to come up with something better. I also thought about whether this could or should be a more generalized VNF, but that at best forces two funcs to be constructed (something like Cast(GcEpoch(byref, epoch))) and seemed more wasteful given how other places also have a single func here already.

[Copilot]

Pull request overview

This PR adjusts JIT value numbering for “cast-away-GC” / unsafe GC-ref-as-integer stores so that identical raw-address snapshots can be CSE’d when considered safe (keyed by a new 2-arg VNFunc).

Changes:

• Add a new value-numbering function VNF_GCRefToPtrWithEpoch for GCRef/Byref-to-raw-address snapshots.
• Update Compiler::fgValueNumberStore to use VNForFunc(..., VNF_GCRefToPtrWithEpoch, <byrefVN>, fgCurMemoryVN[GcHeap]) instead of always generating a unique VN.

Show a summary per file

File	Description
src/coreclr/jit/valuenumfuncs.h	Adds the new VNF_GCRefToPtrWithEpoch VNFunc definition and documentation.
src/coreclr/jit/valuenum.cpp	Uses the new VNFunc in fgValueNumberStore to enable CSE when the epoch argument matches.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 2

[Copilot]

Copilot's findings

Suppressed comments (4)

src/coreclr/jit/valuenum.cpp:14937

• The GC-epoch only advances in fgValueNumberCall, but other IR nodes can introduce GC safe points later (e.g., GT_STORE_BLK lowering to CORINFO_HELP_MEMCPY is explicitly called out as a GC-safe point in lower.cpp). If two cast-away-GC stores are separated by such a node (or a struct store that later lowers to a helper call), fgCurGcEpochVN won’t advance and CSE could still reuse a stale raw-address snapshot across a compacting GC.

    // Advance the GC safepoint epoch at every call that is not in a NoGC region.
    // Allocations and pure arithmetic helpers are GC safepoints but do not mutate
    // the heap, so fgMutateGcHeap is not called for them; the separate epoch VN
    // ensures cast-away-GC address snapshots are not CSE'd across such calls.
    bool isNoGCCall = call->IsHelperCall() && s_helperCallProperties.IsNoGC(call->GetHelperNum());

src/coreclr/jit/valuenumfuncs.h:38

• The comment for VNF_GCRefToPtrWithEpoch says the epoch advances at calls where IsNoGC() is false, “including allocations and pure arithmetic helpers”. NoGC helpers are specifically excluded from potential GC safe points elsewhere (see IsPotentialGCSafePoint), so listing “pure arithmetic helpers” here is misleading/inconsistent.

ValueNumFuncDef(GCRefToPtrWithEpoch, 2, false, false)   // A GC ref/byref reinterpreted as a raw address (BYREF/REF -> I_IMPL).
                                                                //   Args: 0: Source GC ref/byref VN.
                                                                //         1: GC safepoint epoch VN (advances at every call for which IsNoGC() is false,
                                                                //            including allocations and pure arithmetic helpers).
                                                                //   Two stores from the same ref in the same epoch get identical VNs
                                                                //   (CSE is safe; no compacting GC can occur between them).

src/coreclr/jit/valuenum.cpp:14936

• The comment says “Allocations and pure arithmetic helpers are GC safepoints”. NoGC helpers are treated as not being potential GC safe points elsewhere in the JIT (e.g., IsPotentialGCSafePoint), so calling them safepoints here is misleading.

    // Advance the GC safepoint epoch at every call that is not in a NoGC region.
    // Allocations and pure arithmetic helpers are GC safepoints but do not mutate
    // the heap, so fgMutateGcHeap is not called for them; the separate epoch VN
    // ensures cast-away-GC address snapshots are not CSE'd across such calls.

src/coreclr/jit/compiler.h:6563

• The field comment says “allocations and pure arithmetic helpers are GC safepoints”, but NoGC helpers are treated as not being GC safe points elsewhere (e.g., IsPotentialGCSafePoint). This comment should match that definition to avoid confusion.

    // A VN that advances at every GC safepoint (any call for which IsNoGC() is false).
    // This is distinct from fgCurMemoryVN[GcHeap], which only advances on heap mutations;
    // allocations and pure arithmetic helpers are GC safepoints but do not mutate the heap.
    // Used as the epoch argument to VNF_GCRefToPtrWithEpoch to prevent CSE of cast-away-GC
    // address snapshots across GC safepoints where the GC could compact the heap.

• Files reviewed: 4/4 changed files
• Comments generated: 0 new

[jakobbotsch]

This does not look correct to me, I don't see how it would work for fully interruptible functions. Also the diffs do not look like they recover anything.

Are you actually sure the regressions from the PR had anything to do with CSE? I would not expect any CSE to be kicking in for trivial stores like that. Maybe it was copy prop, but that also does not make much sense to me.

[tannergooding]

Are you actually sure the original examples had anything to do with CSE? I would not expect any CSE to be kicking in for trivial stores like that. Maybe it was copy prop, but that also does not make much sense to me.

The repro was toggleable via the CSE knob and would show up in the JITDump.

I had rooted it to non-unique VNs related to the pinned values and it then it being reused across the two fixed boundaries, it was causing this codegen:

str x19, [fp,#0x18]     ; pin #1 established
add x20, x0, w1, UXTW   ; pEnd computed under pin #1
str xzr, [fp,#0x18]     ; pin #1 RELEASED
blr Blocker             ; GC may move the array here
str x19, [fp,#0x18]     ; pin #2 -- new address
mov x0, x19             ; p    = relocated byref
mov x1, x20             ; pEnd = STALE pre-move address

Given something like:

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void Check(byte* p, byte* pEnd, int length)
    {
        if ((pEnd - p) != length)
        {
            if (s_bad++ == 0)
            {
                s_p = p;
                s_pEnd = pEnd;
            }
        }
    }
 
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void Blocker()
    {
        s_garbage = new byte[8192];
        GC.Collect(2, GCCollectionMode.Forced, blocking: true, compacting: true);
    }
 
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void Body(ReadOnlySpan<byte> s)
    {
        fixed (byte* p = &MemoryMarshal.GetReference(s)) { Check(p, p + s.Length, s.Length); }
        Blocker();
 
        fixed (byte* p = &MemoryMarshal.GetReference(s)) { Check(p, p + s.Length, s.Length); }
        Blocker();
 
        fixed (byte* p = &MemoryMarshal.GetReference(s)) { Check(p, p + s.Length, s.Length); }
    }

This does not look correct to me, I don't see how it would work for fully interruptible functions.

Hmmm... Wouldn't that then also be problematic for the other cases that are tracking the GC memory epoch info as part of their VNs?

Also the diffs do not look like they recover anything.

Indeed, this is much worse than the initial local numbers I had gotten. Seems to have dropped almost all the winnings when I handled the fixed() {} alloc fixed() { } case that copilot called on in review.

[jakobbotsch]

The repro was toggleable via the CSE knob and would show up in the JITDump.

I edited my comment, it was confusing, I meant the regressions from the PR.

Hmmm... Wouldn't that then also be problematic for the other cases that are tracking the GC memory epoch info as part of their VNs?

Do you mean the memory VNs, e.g. fgCurMemoryVN?
The difference is that we normally do not need to pay attention to the possibility of relocation. The underlying value of a byref actually may be changing because of relocation, but since any other pointer to it also gets relocated that generally does not matter for VNs model of the heap.

This case is an exception though, modelling that is quite hard in VN (probably not something we should be trying to do, it's a bit too early phase-wise to try to reason about whether a GC could have happened).

[tannergooding]

I edited my comment, it was confusing, I meant the regressions from the PR.

Ah, I guess I didn't dig too much into them. I'll take a deeper look...

What I had looked at mostly seemed to be cases where the conservative fix was causing an additional register to register copy with even fewer being memory to register copies (loads that had otherwise been merged and reused).

The difference is that we normally do not need to pay attention to the possibility of relocation. The underlying value of a byref actually may be changing because of relocation, but since any other pointer to it also gets relocated that generally does not matter for VNs model of the heap.

Ok, that makes sense.

This case is an exception though, modelling that is quite hard in VN (probably not something we should be trying to do, it's a bit too early phase-wise to try to reason about whether a GC could have happened).

That also makes sense. It's also tricky because you can have cases like byte* p = ... and then a secondary local pEnd = p + length and that's really where the problems came in here, because there is no "boundary" for a fixed statement in IL like you see in C#

At best we have a case where the "end" is marked by pinned_lcl = zero (which Roslyn emits if there's any statement after the fixed, but not if the fixed terminates with the method).

So I guess we could try to track that, but it seems tricky and I'm not sure the minor regressions here (especially with most looking like register to register move) is worth it.