JIT: fix issue with out of order importer spilling around some calls #95539
Conversation
If we have a call that returns a struct that is immediately assigned to some local, the call is a GDV inline candidate, and the call is invoked with a copy of the same local on the evaluation stack, the spill of that local into a temp will appear in the IR stream between the call and the ret expr, instead of before the call. As part of our IR resolution the store to the local gets "sunk" into the call as the hidden return buffer, so the importer ordering is manifestly incorrect: ``` call &retbuf, ... tmp = retbuf ...ret-expr ...tmp ``` For normal inline candidates this mis-ordering gets fixed up either by swapping the call back into the ret expr's position, or for successful inlines by swapping the return value store into the ret expr's position. The JIT has behaved this way for a very long time, and the transient mis-ordering has not lead to any noticble problem. For GDV calls the return value stores are done earlier, just after the call, and so the spill picks up the wrong value. GDV calls normally only happen with PGO data. This persistent mis-ordering has been the behavior since at least 6.0, but now that PGO is enabled by default a much wider set of programs are at risk of running into it. The fix here is to reorder the IR in the importer at the point the store to the local is appended to the IR stream, as we are processing spills for the local. If the source of the store is a GT_RET_EXPR we keep track of these spills, find the associated GT_CALL statement, and move the spills before the call. There was a similar fix made for boxes in dotnet#60335, where once again the splitting of the inline candidate call and the subsequent modification of the call to write directly to the return buffer local led to similar problems with GDV calls. Fixes dotnet#95394.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsIf we have a call that returns a struct that is immediately assigned to some local, the call is a GDV inline candidate, and the call is invoked with a copy of the same local on the evaluation stack, the spill of that local into a temp will appear in the IR stream between the call and the ret expr, instead of before the call. As part of our IR resolution the store to the local gets "sunk" into the call as the hidden return buffer, so the importer ordering is manifestly incorrect: For normal inline candidates this mis-ordering gets fixed up either by swapping the call back into the ret expr's position, or for successful inlines by swapping the return value store into the ret expr's position. The JIT has behaved this way for a very long time, and the transient mis-ordering has not lead to any noticble problem. For GDV calls the return value stores are done earlier, just after the call, and so the spill picks up the wrong value. GDV calls normally only happen with PGO data. This persistent mis-ordering has been the behavior since at least 6.0, but now that PGO is enabled by default a much wider set of programs are at risk of running into it. The fix here is to reorder the IR in the importer at the point the store to the local is appended to the IR stream, as we are processing spills for the local. If the source of the store is a GT_RET_EXPR we keep track of these spills, find the associated GT_CALL statement, and move the spills before the call. There was a similar fix made for boxes in #60335, where once again the splitting of the inline candidate call and the subsequent modification of the call to write directly to the return buffer local led to similar problems with GDV calls. Fixes #95394.
|
|
@EgorBo PTAL No diffs locally, verified the repro fails w/o the fix and works with, likewise with Egor's repro and the original one from the bug. |
|
/azp run runtime-coreclr jitstress, Fuzzlyn, runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| for (int i = 0; i < 100; i++) | ||
| { | ||
| _ = Problem(p, n, (-1, -1)); | ||
| Thread.Sleep(30); |
There was a problem hiding this comment.
The repro needs Tiered Compilation, right? Then I presume you need to run some pipeline where it's not disabled (like pgo ones) (or add TC=1 in csproj)
There was a problem hiding this comment.
Yes, this needs to have a GDV expansion to expose the bug.
|
/azp run runtime-coreclr pgo, runtime-coreclr pgostress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/backport to release/8.0-staging |
|
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7088765976 |
If we have a call that returns a struct that is immediately assigned to some local, the call is a GDV inline candidate, and the call is invoked with a copy of the same local on the evaluation stack, the spill of that local into a temp will appear in the IR stream between the call and the ret expr, instead of before the call.
As part of our IR resolution the store to the local gets "sunk" into the call as the hidden return buffer, so the importer ordering is manifestly incorrect:
For normal inline candidates this mis-ordering gets fixed up either by swapping the call back into the ret expr's position, or for successful inlines by swapping the return value store into the ret expr's position. The JIT has behaved this way for a very long time, and the transient mis-ordering has not lead to any noticble problem.
For GDV calls the return value stores are done earlier, just after the call, and so the spill picks up the wrong value. GDV calls normally only happen with PGO data. This persistent mis-ordering has been the behavior since at least 6.0, but now that PGO is enabled by default a much wider set of programs are at risk of running into it.
The fix here is to reorder the IR in the importer at the point the store to the local is appended to the IR stream, as we are processing spills for the local. If the source of the store is a GT_RET_EXPR we keep track of these spills, find the associated GT_CALL statement, and move the spills before the call.
There was a similar fix made for boxes in #60355, where once again the splitting of the inline candidate call and the subsequent modification of the call to write directly to the return buffer local led to similar problems with GDV calls.
Fixes #95394.