JIT: fix double reporting of some GC frame slots

[AndyAyersMS]

If there is a gc struct local that is dependently promoted, the struct
local may be untracked while the promoted gc fields of the struct are
tracked.

If so, the jit will double report the stack offset for the gc field,
first as an untracked slot, and then as a tracked slot.

Detect this case and report the slot as tracked only.

Closes #71005.

[ghost]

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

Issue Details

---

If there is a gc struct local that is dependently promoted, the struct
local may be untracked while the promoted gc fields of the struct are
tracked.

If so, the jit will double report the stack offset for the gc field,
first as an untracked slot, and then as a tracked slot.

Detect this case and report the slot as tracked only.

Closes #71005.

Author:	AndyAyersMS
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[AndyAyersMS]

@dotnet/jit-contrib PTAL

Not sure yet why this hasn't bitten us before, or (perhaps) only seems to have caused arm64 OSR failures.

This bit of GC reporting has not changed in many many years.

[AndyAyersMS]

This seems to be the wrong approach. During genFnProlog we scan for on-frame locals that will require tracked reporting, and explicitly exclude dependently promoted structs. However, we keep track of these locals by a pair of frame offsets.

So evidently something goes wrong here (perhaps just with OSR) and we get this range wrong, or are not properly ordering the frame so that we're intermingling locals improperly or something. Continuing to investigate.

[SingleAccretion]

@AndyAyersMS FWIW, I investigated this earlier today, perhaps my conclusions could be of help.

Details (note these are purely from looking at the code and may be partially or completely incorrect.)

Ordinarily the frame allocation is done s.t. the "tracked GC refs on stack" are all contained in one region:

If we're supposed to track lifetimes of pointer temps, we'll
assign frame offsets in the following order:

    non-pointer local variables (also untracked pointer variables)
        pointer local variables
        pointer temps
    non-pointer temps

The way reporting of "tracked GC refs on stack" is done, codegen calculates the low/high offsets within which they reside:

runtime/src/coreclr/jit/codegencommon.cpp

Lines 5634 to 5654 in 6bc83f1

	/* We need to know the offset range of tracked stack GC refs */
	/* We assume that the GC reference can be anywhere in the TYP_STRUCT */
	if (varDsc->HasGCPtr() && varDsc->lvTrackedNonStruct() && varDsc->lvOnFrame)
	{
	// For fields of PROMOTION_TYPE_DEPENDENT type of promotion, they should have been
	// taken care of by the parent struct.
	if (!compiler->lvaIsFieldOfDependentlyPromotedStruct(varDsc))
	{
	hasGCRef = true;
	if (loOffs < GCrefLo)
	{
	GCrefLo = loOffs;
	}
	if (hiOffs > GCrefHi)
	{
	GCrefHi = hiOffs;
	}
	}
	}

So, ordinarily, we would not have any dependently promoted fields in this range (since they'd be in the "untracked" group).

Presumably, OSR violates this by virtue of having "fixed" offsets for some locals.

When the emitter looks at which of these slots actually need to be reported here:

runtime/src/coreclr/jit/emit.cpp

Lines 6543 to 6555 in 6bc83f1

	/* Are tracked stack ptr locals laid out contiguously?
	If not, skip non-ptrs. The emitter is optimized to work
	with contiguous ptrs, but for EditNContinue, the variables
	are laid out in the order they occur in the local-sig.
	*/
	if (!emitContTrkPtrLcls)
	{
	if (!emitComp->lvaIsGCTracked(dsc))
	{
	continue;
	}
	}

It calls lvaIsGCTracked that will always return false for dependently promoted fields on x64, but not ARM64 (note emitContTrkPtrLcls is always false on both targets).

[AndyAyersMS]

@SingleAccretion thanks, I have been looking at exactly this area.

Seems like there are two options:
(1) change filtering I'm doing now to use lvaIsGCTracked to filter out some cases where despite being a tracked GC ref, it is reported as untracked;
(2) change the call to emitEndCodeGen so that for OSR we don't claim to have a contiguous range of tracked stack locals (which ends up calling into lvaIsGCTracked under the covers). Actually (as you noted) we're already doing that for arm64/x64, so seems like it would not be sufficient.

[SingleAccretion]

Judging by the code which binds the offset range for "tracked on-stack GC refs", it seems to me that "dependently promoted fields" cannot really be tracked, otherwise we would need to include them into the range.

[AndyAyersMS]

For OSR the tracked range may end up including some trackable dependent promoted locals. They won't ever inspire or extend the range, but they may fall within the range set by other on frame gc vars.

I updated the code to detect this case and report as tracked, by querying the emitter to see if the offset falls within the tracked range. I suppose there's still a concern that we might not properly track these lifetimes (since say they can have aliased reads/writes via the parent) in which case we'd need to find a way to exclude them from tracked reporting somehow.

[AndyAyersMS]

Was hoping to scope this down to just an OSR change, but am seeing some non-OSR hits for this new check:

;; arm64 windows crossgen of SPC
;; System.IO.Enumeration.FileSystemEnumerator`1:InternalDispose(bool):this

1 tracked GC refs are at stack offsets  0010 ...  0018

;  V05 tmp1         [V05    ] (  2,  0   )  struct (24) [fp+18H]   do-not-enreg[HS] must-init hidden-struct-arg "struct address for call/obj"
;* V06 tmp2         [V06    ] (  0,  0   )     ref  ->  zero-ref    class-hnd "Inlining Arg"
;  V07 tmp3         [V07,T04] (  3,  0   )    long  ->   x0         "Inlining Arg"
;* V08 tmp4         [V08    ] (  0,  0   )    long  ->  zero-ref    "Inline stloc first use temp"
;  V09 tmp5         [V09,T06] (  1,  0   )     ref  ->  [fp+18H]   do-not-enreg[H] hidden-struct-arg V05.Item2(offs=0x00) P-DEP "field V05.Item2 (fldOffset=0x0)"
;  V10 tmp6         [V10,T05] (  2,  0   )    long  ->  [fp+20H]   do-not-enreg[H] hidden-struct-arg V05.Item1(offs=0x08) P-DEP "field V05.Item1 (fldOffset=0x8)"
;  V11 tmp7         [V11,T07] (  1,  0   )     int  ->  [fp+28H]   do-not-enreg[H] hidden-struct-arg V05.Item3(offs=0x10) P-DEP "field V05.Item3 (fldOffset=0x10)"
;  V12 PSPSym       [V12,T02] (  1,  1   )    long  ->  [fp+38H]   do-not-enreg[V] "PSPSym"

Untracked GC struct slot V05+0 (P-DEP promoted V09) is at frame offset 24 within tracked ref range.

In this case V09 only appears as a child of its parent and never becomes live. Looks like the parent is an ignored gc struct return value.

Or maybe I need to exclude the upper end of the offset range. That's probably it.

[AndyAyersMS]

Looks like may be arm with profiler callbacks ends up tripping this; likely need to exempt "prespilled" cases too.

[AndyAyersMS]

/azp run runtime-coreclr libraries-pgo

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[AndyAyersMS]

PTAL @dotnet/jit-contrib

Libraries-pgo has some known failures so I didn't expect it to run cleanly.

[SingleAccretion]

I suppose there's still a concern that we might not properly track these lifetimes

Yes... I think some unrolled varieties of block copies, especially those using SIMD registers, do not properly report GC-ness for the destination stack slots. Likewise for our special calls with tracked "return buffers".

[AndyAyersMS]

I suppose there's still a concern that we might not properly track these lifetimes

Yes... I think some unrolled varieties of block copies, especially those using SIMD registers, do not properly report GC-ness for the destination stack slots. Likewise for our special calls with tracked "return buffers".

Ok, I've switched things around to always report the fields of dependently promoted GC structs as untracked.

[SingleAccretion]

LGTM modulo nits.

[AndyAyersMS]

/azp run runtime-coreclr libraries-pgo

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[AndyAyersMS]

Was hoping this plus other recent PRs would have cleaned up libraries-pgo, but there is still more going on there. Going to see if the new slew of arm64 failures repro locally (looks like a machine issue but I can't tell for sure).

[AndyAyersMS]

Kicked off a new libraries-pgo baseline to see what that looks like.

[AndyAyersMS]

libraries-pgo picture is still a bit confusing but I think this change is good.