JIT: Avoid removing multi-use boxes #72842
Conversation
There was an assumption that the local operand to a GT_BOX node is single-use which was being violated when GDV clones these as arguments. We now allow multi-uses of these locals by setting a flag when cloning and then handle it in gtTryRemoveBoxUpstreamEffects. There is also GTF_VAR_CLONED that would be set on the local itself, but given that transformations can affect the operand local node arbitrarily I went with another of these type of flags on GT_BOX instead. Fix dotnet#72775
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThere was an assumption that the local operand to a GT_BOX node is There is also GTF_VAR_CLONED that would be set on the local itself, but Fix #72775
|
|
Sort of a shame that we do not get rid of the box in the hot path. I guess in general that would require some partial redundancy elimination, or the more ad-hoc fix is to have GDV move and clone the setup statements for arguments that are boxes. cc @dotnet/jit-contrib PTAL @AndyAyersMS |
| @@ -543,6 +543,7 @@ enum GenTreeFlags : unsigned int | |||
| GTF_QMARK_CAST_INSTOF = 0x80000000, // GT_QMARK -- Is this a top (not nested) level qmark created for | |||
| // castclass or instanceof? | |||
|
|
|||
| GTF_BOX_CLONED = 0x40000000, // GT_BOX -- this box and its operand has been cloned, cannot assume it to be single-use anymore | |||
There was a problem hiding this comment.
The local under the box is also marked as "cloned" -- is a new flag truly needed?
There was a problem hiding this comment.
Transformations can affect that local in arbitrary ways (I mentioned it above).
There was a problem hiding this comment.
Hmm, yes, sorry I missed the description somehow.
I suppose this is more robust in the face of something like:
var sdsuTmp = boxTmp; // Cloned.
Use(BOX(sdsuTmp));So it is needed.
There was a problem hiding this comment.
Right, something like that. It would probably be ok in practice, especially since gtTryRemoveBoxUpstreamEffects is only called very early, but given the function name it seems best to make it more robust.
|
Failure is #72879 |
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/2755682979 |
There was an assumption that the local operand to a GT_BOX node is
single-use which was being violated when GDV clones these as arguments.
We now allow multi-uses of these locals by setting a flag when cloning
and then handle it in gtTryRemoveBoxUpstreamEffects.
There is also GTF_VAR_CLONED that would be set on the local itself, but
given that transformations can affect the operand local node arbitrarily
I went with another of these type of flags on GT_BOX instead.
Fix #72775