-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
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* Compiler::fgMakeMultiUse(GenTree** pOp, CORINFO_CLASS_HANDLE structType /*= nullptr*/) | ||
{ | ||
GenTree* const tree = *pOp; | ||
|
||
if (fgIsSafeToClone(tree)) | ||
if (tree->IsInvariant() || tree->OperIsLocal()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does a simple IsLeaf
work here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, and a great catch as always by the fuzzer. :)
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.