JIT: run local struct to field update in morph both pre and post order for returns #75304
Conversation
…to field In some rare cases (currently only jitstress) morph may miss out in updating a return of a "struct that can be replaced by its field" with the field, because we run local assertion prop afterward we check for this update, and assertion prop, and it may change the local from one that could not be updated into one that could be. So do a special run of assertion prop before this update, so that the local we analyze for possible update is same the one we'll be using in the end. I tried moving all of this to post-order so local assertion prop only ran once but that lead to more diffs and some regressions. Fixes dotnet#74900.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue Details…to field In some rare cases (currently only jitstress) morph may miss out in updating a return of a "struct that can be replaced by its field" with the field, because we run local assertion prop afterward we check for this update, and assertion prop, and it may change the local from one that could not be updated into one that could be. So do a special run of assertion prop before this update, so that the local we analyze for possible update is same the one we'll be using in the end. I tried moving all of this to post-order so local assertion prop only ran once but that lead to more diffs and some regressions. Fixes #74900.
|
|
@jakobbotsch PTAL No diffs (prior commit had minor diffs). |
src/coreclr/jit/morph.cpp
Outdated
| while (newOp1 != nullptr) | ||
| { | ||
| op1 = newOp1; | ||
| newOp1 = optAssertionProp(apFull, newOp1, nullptr, nullptr); |
There was a problem hiding this comment.
It would feel like a lesser evil to "retry" replacing the local with its promoted field in post-order to me. It would be more in line with how the other decomposition transforms work.
There was a problem hiding this comment.
I agree it seems like a more natural fit.
I had a version that did that, but it caused LSRA churn (though not too much).
There was a problem hiding this comment.
I had a version that did that, but it caused LSRA churn (though not too much).
It seems a bit surprising it did -- wouldn't the lack of this replacement cause the failure (assert) being fixed?
To be clear, I meant trying the replacement on both pre- and post- order.
There was a problem hiding this comment.
Yes, I was trying it both places. I'll revive that version....
There was a problem hiding this comment.
Looks like my earlier assessment was wrong; doing the update both pre and post order works nicely and has no diffs on win x64 or x86.
Updated.
|
Native AOT tests timed out, seems like this is #75005 with a fix forthcoming. |
AndyAyersMS
changed the title
|
@jakobbotsch ping |
ghost
locked as resolved and limited conversation to collaborators
In some rare cases (currently only jitstress) morph may miss out in updating a return of a "struct that can be replaced by its field" with the field. But because we run local assertion prop after we check for this update, assertion prop it may change the local from one that could not be updated into one that could be, and if this happens, we miss out on a (required) update.
So, retry the update in post order, after the return operand has been morphed.
Fixes #74900.