JIT: Avoid reordering operands in fgMorphModToSubMulDiv #71615
Conversation
fgMorphModToSubMulDiv tries to check if it is ok to "clone" locals instead of spilling them by checking for address exposure. This was necessary because GTF_GLOB_REF is not up-to-date in preorder during morph, which is when this runs. However, address exposure is not valid for implicit byrefs at this point, so the check is not enough. The logic is trying to figure out which of the operands need to be spilled and which of them can be cloned. However, the logic was also wrong in the face of potential embedded assignments. Thus, rework it to be a bit more conservative but correct. As part of this, remove fgIsSafeToClone and make fgMakeMultiUse less conservative by avoiding checking for address exposure. This was probably an attempt at some limited interference checks, but from what I could see other uses do not need this. Fix dotnet#65118
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsfgMorphModToSubMulDiv tries to check if it is ok to "clone" locals The logic is trying to figure out which of the operands need to be As part of this, remove fgIsSafeToClone and make fgMakeMultiUse less Fix #65118
|
| GenTree* const tree = *pOp; | ||
|
|
||
| if (fgIsSafeToClone(tree)) | ||
| if (tree->IsInvariant() || tree->OperIsLocal()) |
There was a problem hiding this comment.
IsInvariant() also checks for addresses of locals which may not be a leaf node. But yes, we could probably clone any leaf or even other side-effect-free trees with the right profitability checks.
There was a problem hiding this comment.
I saw no extra diffs from also cloning all leaves, so will leave this as is (actually, will change the use of gtClone below to gtCloneExpr).
|
cc @dotnet/jit-contrib PTAL @TIHan |
| // not worth it. | ||
| bool spillDividend; | ||
| bool spillDivisor; | ||
| if (divisor->IsInvariant() || divisor->OperIsLocal()) |
There was a problem hiding this comment.
This logic is complicated, but I understand the reasoning behind it. It's a way to reliably handle interference. Luckily, we don't have to do this sort of thing a lot.
TIHan
left a comment
TIHan
left a comment
There was a problem hiding this comment.
This looks good, and a great catch as always by the fuzzer. :)
3 participants
fgMorphModToSubMulDiv tries to check if it is ok to clone locals
instead of spilling them by checking for address exposure. This was
necessary because GTF_GLOB_REF is not up-to-date in preorder during
morph, which is when this runs. However, address exposure is not valid
for implicit byrefs at this point, so the check is not enough.
The logic is trying to figure out which of the operands need to be
spilled and which of them can be cloned. However, the logic was also
wrong in the face of potential embedded assignments. Thus, rework it to
check interference on its own.
As part of this, remove fgIsSafeToClone and make fgMakeMultiUse less
conservative by avoiding checking for address exposure. This was
probably an attempt at some limited interference checks, but other uses do
not seem to need this.
Fix #71600
Few minor diffs and a couple of large ones in some huge tests with thousands of locals that now end up needing a reserved register to form stack frame offsets. I'm not worried about those.