Fix evaluation order for block copy #19334
Conversation
|
@dotnet-bot test Ubuntu jitstress2 |
|
I obviously don't have the syntax right for the stress tests, but PMI diffs exposed an assert in a pri 1 test, so I'll work on that, and then when I re-push I'll get the right syntax to do stress testing. |
src/jit/gentree.cpp
Outdated
| // However, if op1 is a lclVar address, the side-effect flags don't really apply in | ||
| // this situation (in fact, they are quite likely overly conservative; otherwise they | ||
| // reflect the fact that an address-exposed local is being assigned to), | ||
| // and SSA depends on having op2 evaluated first if the destination is a local. |
There was a problem hiding this comment.
I'm not entirely comfortable with this. It appears that the ancient comment from Compiler::fgSetTreeSeqHelper() (that I mention in one of my comments on the original bug - #19243) still applies for SSA. It is really unfortunate that we consider the assignment to take place on the lcl node itself, instead of at the assignment, which is how it ought to be. The ASG representation is a fundamental problem. That said, I believe that this is correct but would like to have some other JIT-knowledge applied to asses this: @dotnet/jit-contrib @mikedn
There was a problem hiding this comment.
Hmm, I don't quite understand the comment. If an address-exposed local is being assigned to how is SSA involved in this?
There was a problem hiding this comment.
I guess I need to clarify the comment. What I meant to say was that if we have a lclVar assignment (i.e. the lhs is an indirection of an address involving a lclVar) that is marked with one of the GTF_ALL_EFFECT flags, either 1) it is simply overly conservative (this case, where we overly aggressively apply the GLOB_REF flag to FIELD nodes), or 2) it is address-taken. In either case, if we know that the lhs is a lclVar address, then it is safe to evaluate the rhs first, as the side effects on the lhs only apply to the actual assignment.
There was a problem hiding this comment.
that is marked with one of the GTF_ALL_EFFECT flags, either 1) it is simply overly conservative (this case, where we overly aggressively apply the GLOB_REF flag to FIELD nodes), or 2) it is address-taken
Yes, I understand that, it's just the SSA bit that I find confusing. I guess that what you're saying is that it doesn't impact SSA, exactly because it's an address taken variable which SSA will just ignore.
There was a problem hiding this comment.
The code seems correct. Though I wish the JIT wouldn't do this dance of having the address of a lclvar being taken and then immediately assign to it through an indir. Such cases should be morphed to assign directly to the lclvar (of course, that wouldn't probably work for dynblk but that's a special and rare case).
There was a problem hiding this comment.
Yes, I totally agree. Would it be clearer if the comment said:
// However, if the LHS is a lclVar address, SSA relies on using evaluation order for its
// renaming, and therefore the RHS must be evaluated first.
// If we have an assignment involving a lclVar address, the LHS may be marked as having side-effects.
// However the side-effects won't require that we evaluate the LHS address first:
// - The GTF_GLOB_REF might have been conservatively set on a FIELD of a local.
// - The local might be address-exposed, but that side-effect happens at the actual assignment (not
// when its address is "evaluated") so it doesn't change the side effect to "evaluate" the address
// after the RHS (note that in this case it won't be renamed by SSA anyway, but the reordering is safe).
|
@dotnet-bot help |
|
@dotnet-bot test Windows_NT x64 Checked jitstress2_jitstressregs8 |
|
@dotnet-bot test Windows_NT x64 Checked jitstress2_jitstressregs8 |
|
@dotnet-bot test Ubuntu x64 Checked jitstress2 |
|
The Ubuntu stress job was "aborted" for some unexplained reason. |
This order was changed five years ago, as a workaround for data flow analysis issues around the block copy. Now that block copies are handled as regular assignments, this is no longer needed - and is, in fact, incorrect as it causes side effects to be handled in the wrong order. However, for the case where the lhs is an indirection of a local address, it must be evaluated 2nd for SSA renaming to be correct. In the process, also remove the special-casing for the size operand of `DYN_BLK` under `ASG`. Fix #19243
|
@dotnet-bot test OSX10.12 x64 Release CoreFX Tests |
|
@dotnet-bot test Ubuntu x64 Checked jitstress2 |
|
@dotnet-bot test OSX10.12 x64 Debug CoreFX Tests |
|
Re-testing the OSX10.12 x64 Debug CoreFX leg. The other two stress legs appear to be known failures. @AndyAyersMS approved this, but that was before I realized that we need to ensure that the order is reversed if there's a local addr on the lhs. Could you have a look at the new gentree.cpp changes? Or perhaps someone else @dotnet/jit-contrib could take a look. |
I have not seen this job green. I think it was broken from the moment when it was added and ci history shows the same. |
|
@dotnet/jit-contrib ping |
src/jit/flowgraph.cpp
Outdated
| if (dynBlk->gtEvalSizeFirst) | ||
| { | ||
| fgSetTreeSeqHelper(sizeNode, isLIR); | ||
| } | ||
| if (tree->gtFlags & GTF_REVERSE_OPS) | ||
| if (isReverse && (src != nullptr)) |
There was a problem hiding this comment.
What does it mean when src is a nullptr?
Maybe add a comment explaining what it means.
This order was changed five years ago, as a workaround for data flow analysis issues around the block copy. Now that block copies are handled as regular assignments, this is no longer needed - and is, in fact, incorrect as it causes side effects to be handled in the wrong order.
Fix #19243