JIT: fgMorphModToSubMulDiv should not reorder operands #65118
Conversation
There was existing logic to spill but it was there to avoid duplicating costly trees and was not considering possible interference. Unfortunately, this method is invoked in morph preorder and child node flags can't be trusted for interference checks. So, just always spill. Doing so lets us clean up some logic later on that needed set reverse ops. Fixes dotnet#65104.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsThere was existing logic to spill but it was there to avoid duplicating costly trees and was So, just always spill. Doing so lets us clean up some logic later on that needed set reverse ops. Fixes #65104.
|
|
@jakobbotsch PTAL If this ends up causing bad diffs we can see if we can do some ad-hoc analysis to avoid spilling unaliased locals. |
Looks like quite a bit more regression than I would have expected. Will need to look deeper. Some of this may be inevitable given safety constraints. |
|
Also seeing a few instances of Arm64 hitting So could be something in rationalize needs looking into as well. |
|
Updated to minimize diffs. |
|
Finally realized yes, we need reverse ops, as we are putting the comma ops in the op2 subtree of the sub. So put that bit back in with a hopefully clearer comment. |
|
New diffs look reasonable. |
ghost
locked as resolved and limited conversation to collaborators
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
* JIT: Avoid reordering operands in fgMorphModToSubMulDiv 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 #65118 * Add a test * Revert unnecessary change * Switch to gtCloneExpr
There was existing logic to spill but it was there to avoid duplicating costly trees and was
not considering possible interference. Unfortunately, this method is invoked in morph
preorder and child node flags can't be trusted for interference checks.
So, just always spill.
Doing so lets us clean up some logic later on that needed set reverse ops.
Fixes #65104.