JIT: Unify and clean up unspilling

[jakobbotsch]

No description provided.

[ghost]

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

Issue Details

---

null

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

[jakobbotsch]

/azp run runtime-coreclr superpmi-replay, runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jakobbotsch]

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @BruceForstall

This is the original ("risky") change I had around unspilling. A few diffs, mostly on arm32 that now has the same behavior as other platforms. Also a handful of win-x86 diffs due to different GC reporting -- in particular when we had a TYP_BYREF LclVarDsc, but a TYP_I_IMPL LCL_VAR -- the previous logic would unspill as TYP_I_IMPL, the new logic unspills as TYP_BYREF. I do not think this should have any adverse effects (and I am not totally sure how we end up with these LCL_VAR type mismatches, actually).

[jakobbotsch]

ping @BruceForstall

[BruceForstall]

in particular when we had a TYP_BYREF LclVarDsc, but a TYP_I_IMPL LCL_VAR -- the previous logic would unspill as TYP_I_IMPL, the new logic unspills as TYP_BYREF

Don't we need to respect the type on the tree, not the LclVarDsc? Can you get this situation using Unsafe.As? Isn't it ok to "lose" gc/byref pointers (in a pinned context?) but not the other way around (I_IMPL -> BYREF)?

[jakobbotsch]

Don't we need to respect the type on the tree, not the LclVarDsc? Can you get this situation using Unsafe.As? Isn't it ok to "lose" gc/byref pointers (in a pinned context?) but not the other way around (I_IMPL -> BYREF)?

I don't think we can get in this situation with Unsafe.As, we should not be folding GC pointer<->non GC pointer conversions like this. The place that seems to produce these mistypings seems to be block morph, I've opened #92292 to fix that.

In general I don't think we need to care much about unspilling here -- I went for the most conservative thing in this PR (keep it tracked if either are tracked, otherwise don't). I am quite sure the usual case, where there is no unspill, just ignores the new untracked type. I.e. if we saw a LCL_VAR<BYREF>(V00) use followed by a LCL_VAR<I_IMPL>(V00) use, we would not change GC reporting from tracked to untracked. If we don't respect it for the normal case I don't think we need to respect it for the unspilling case.

Regardless, let's see if there are more cases left after #92292, and if not I can add an assert and get rid of the extra code I added. I think it would be nice to have the invariant that we don't have mistyped LCL_VAR nodes (except for the NOL/small locals cases).

[SingleAccretion]

@jakobbotsch there are a few more cases where BYREF<->I_IMPL mismatches arise. One is inliner retyping (integer -> byref), another is in importer stack value unification (byref -> integer).

[jakobbotsch]

@jakobbotsch there are a few more cases where BYREF<->I_IMPL mismatches arise. One is inliner retyping (integer -> byref), another is in importer stack value unification (byref -> integer).

Do you have an example of the latter? I don't see any LCL_VAR<TYP_I_IMPL> for TYP_BYREF locals over SPMI with #92292. I do see the other way around, but this PR should handle those in the same way as previously.

[SingleAccretion]

Do you have an example of the latter?

I recall it should happen around here:

runtime/src/coreclr/jit/importer.cpp

Lines 11428 to 11435 in a7cafec

	if (genActualType(tree->gtType) == TYP_BYREF && lvaTable[tempNum].lvType == TYP_INT)
	{
	// Some other block in the spill clique set this to "int", but now we have "byref".
	// Change the type and go back to re-import any blocks that used the wrong type.
	lvaTable[tempNum].lvType = TYP_BYREF;
	reimportSpillClique = true;
	}
	else if (genActualType(tree->gtType) == TYP_INT && lvaTable[tempNum].lvType == TYP_BYREF)

Last I ran this through SPMI, the case was hit in some runtime tests on x86.

It's why the assert here:

runtime/src/coreclr/jit/lclvars.cpp

Lines 4147 to 4151 in a7cafec

	// Check that the LCL_VAR node has the same type as the underlying variable, save a few mismatches we allow.
	assert(tree->TypeIs(varDsc->TypeGet(), genActualType(varDsc)) ||
	(tree->TypeIs(TYP_I_IMPL) && (varDsc->TypeGet() == TYP_BYREF)) || // Created for spill clique import.
	(tree->TypeIs(TYP_BYREF) && (varDsc->TypeGet() == TYP_I_IMPL)) || // Created by inliner substitution.
	(tree->TypeIs(TYP_INT) && (varDsc->TypeGet() == TYP_LONG))); // Created by "optNarrowTree".

Allows this case.

[jakobbotsch]

Last I ran this through SPMI, the case was hit in some runtime tests on x86.

The code is under #ifdef TARGET_64BIT, but maybe that's why the problem happens on x86 -- the code should probably run on x86 too to ensure we go back and reimport the clique with proper GC-ness. Seems like otherwise it will result in bad GC reporting for this case.

[SingleAccretion]

The code is under #ifdef TARGET_64BIT

Oops, it may have actually been the case just above that runs for all targets:

runtime/src/coreclr/jit/importer.cpp

Lines 11398 to 11403 in a7cafec

	if (tree->gtType == TYP_BYREF && lvaTable[tempNum].lvType == TYP_I_IMPL)
	{
	lvaTable[tempNum].lvType = TYP_BYREF;
	impReimportMarkSuccessors(block);
	markImport = true;
	}

[jakobbotsch]

Ok -- if we do reimport the successors then I don't see how we end up with the mistyped LCL_VAR from there. I would expect/hope the reimportation step to produce correctly typed LCL_VAR nodes for the retyped LclVarDsc... that's sort of the point of it.

[SingleAccretion]

I believe the failure mode was that some already imported block (a predecessor) would not get reimported and use the I_IMPL type for its LCL_VAR.

[jakobbotsch]

I believe the failure mode was that some already imported block (a predecessor) would not get reimported and use the I_IMPL type for its LCL_VAR.

Yeah, seems like we don't revisit the predecessors to fix up the types of the stores into the spill clique temps we have already created.

[AndyAyersMS]

IIRC, the inspiration for most of the reimportation / spill clique behavior was C++/CLI, and we left most of those tests behind on desktop. So not surprising if we don't see it hit much in our current test suite.

[jakobbotsch]

It looks like a pretty trivial oversight -- these mismatches should cause full reimportation of the entire spill clique, not just of successors. All other cases than that particular one will already induce a full reimport. #92307 makes that case do the same.

[jakobbotsch]

@BruceForstall This has no diffs now, except for arm32 diffs from aligning it with the rest of our targets.

[BruceForstall]

LGTM