Kill a non-gc-type lclVar if it has a gc-type value#679
Kill a non-gc-type lclVar if it has a gc-type value#679CarolEidt merged 5 commits intodotnet:masterfrom
Conversation
|
This was exposed by the EH Write-thru work. It is a rare case where we have a re-typed lclVar that is live at a pinvoke. In the current JIT, the case where this occurred involved an EH-exposed lclVar, so it was not a register candidate. |
|
@dotnet/jit-contrib PTAL |
src/coreclr/src/jit/lsra.cpp
Outdated
| // The importer will assign a GC type to the rhs of an assignment if the lhs type is a GC type, | ||
| // even if the rhs is not. See the CEE_STLOC* case in impImportBlockCode(). As a result, | ||
| // we can have an integer type lclVar that behaves as a GC type. In that case we need to | ||
| // kill the local. |
There was a problem hiding this comment.
In that case we need to kill the local
Why? Ideally comments should explain why something is done, not that it's supposed to be done.
There was a problem hiding this comment.
This is because 1) we can't have any gc refs in registers (even callee-saves) during a pinvoke, 2) the emitter will mark the register as having a GC type if the node has a GC type, and 3) then the code generator asserts that there are no GC types in registers (even callee-save) at the pinvoke prolog.
I'll expand the comment - also it probably should say "spill" the local, even though the code describes it as "killing" gc refs.
src/coreclr/src/jit/lsra.cpp
Outdated
| // The importer will assign a GC type to the rhs of an assignment if the lhs type is a GC type, | ||
| // even if the rhs is not. See the CEE_STLOC* case in impImportBlockCode(). As a result, | ||
| // we can have an integer type lclVar that behaves as a GC type. In that case we need to | ||
| // kill the local. |
There was a problem hiding this comment.
Do we need to kill that local because GC will try to add that address as alive or because its value won't be correct after whatever we are spilling for?
Can we improve that and do not spill such locals?
There was a problem hiding this comment.
We need to spill (not really "kill") the local because we don't support GC refs in registers for unmanaged calls. I think that we could possibly improve the handling of stack references, because even though the address of stack locals is generally marked as a byref, we don't really have to report it to the GC if we know it's on the stack. That said, this case is rare enough (I don't think it can be done in C#) that I'm not sure it's worth optimizing for.
|
Is this the importer code you're referring to? runtime/src/coreclr/src/jit/importer.cpp Lines 11216 to 11224 in d2685d5 Do you have a motivating test case you can add? |
|
@AndyAyersMS - Yes, that's the importer code I was referring to. And I didn't spend a lot of time, but was unable to come up with a test case for this. It caused a failure in a test case on arm32 with my EH write-thru changes. |
| continue; | ||
| } | ||
| unassignPhysReg(regRecord, assignedInterval->recentRefPosition); | ||
| bool needsKill = varTypeIsGC(assignedInterval->registerType); |
There was a problem hiding this comment.
Looks like I am missing something, the prev line has:
if (!varTypeIsGC(assignedInterval->registerType)))
continue;
varTypeIsGC(assignedInterval->registerType)) is always true after that, means:
bool needsKill = varTypeIsGC(assignedInterval->registerType); always true.
if (!needsKill) always false, so the code and the comment under that block is never executed.
There was a problem hiding this comment.
That was a mistake in extracting the fix from my EHWriteThru branch to this one. Thanks for catching that!
src/coreclr/src/jit/lsra.cpp
Outdated
| // even if the rhs is not. See the CEE_STLOC* case in impImportBlockCode(). As a result, | ||
| // we can have a 'GT_LCL_VAR' node with a GC type, when the lclVar itself is an integer type. | ||
| // The emitter will mark this register as holding a GC type. Therfore we must spill this value. | ||
| // This was exposed by the Interop\StructMarshalling\ReversePInvoke\MarshalSeqStruct\ |
There was a problem hiding this comment.
// This was exposed by
Nit: I believe that including an exposing test reference in source code is not a common practice. I would leave that in the PR, so people can find that PR (by running git blame/annotate) and find the test reference here if they ever need it.
No description provided.