Fixing assert on variable live range for simd12 LCL / STORE_LCL #79182
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsIntend to fix 78898. Op GT_STORE_LCL_VAR for target type SIMD12 always stores the result on the stack when FEATURE_SIMD is on. Liveness and the variable descriptor were updated after emitting this operation. This PR updates the tree's variable descriptor and liveness. Having said that, LSRA assigned a target register for GT_STORE_LCL_VAR is assigned a target register in LSRA although the result (v01 arg0) is stored on the stack. For that reason, the tree target reg is being updated inside our specific case. LSRA already has the information about the available registers, then might be better to add this SIMD12 information there and avoid changing the tree after. I am opening this PR to see if this is triggering other problems. The same behavior may be happening for GT_STOREIND for the same target type but I cannot get an example.
|
|
/azp run runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I wonder why the arm64 |
src/coreclr/jit/simdcodegenxarch.cpp
Outdated
| @@ -938,10 +938,10 @@ void CodeGen::genStoreLclTypeSIMD12(GenTree* treeNode) | |||
| { | |||
| assert((treeNode->OperGet() == GT_STORE_LCL_FLD) || (treeNode->OperGet() == GT_STORE_LCL_VAR)); | |||
|
|
|||
| treeNode->SetRegNum(REG_STK); // we always store the result on stack | |||
There was a problem hiding this comment.
It seems odd to call this. Isn't the problem that LSRA shouldn't be assigning a register to this node at all?
ghost
added
the
needs-author-action
|
Just a comment I didn't share before. If we simply modify V01 varDsc home to STK (as it is done in other places) and call to update liveness, or just call to the update liveness function directly, the assert won't hit but we would be reporting a home that is not meant to be (xmm1). The liveness function would update its varDsc home to xmm1 anyway and report it to start living there, although STORE_LCL_VAR is doing a spill for this specific case (x86 and type SIMD12). That is why I changed the tree target register. I thought the best was to update the tree target register to REG_NA but @kunalspathak suggests it could also be that we are wrongly not setting the spill flag. |
ghost
removed
the
needs-author-action
|
|
|
/azp run runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Pushed fixes at other places where we were not tracking the liveness of |
|
If jitstress looks ok, we should also kick off gcstress @BrianBohe |
|
/azp run runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Although LSRA is indicating the tree produces the result in an xmm's register, we are storing vector3 always on stack, so its home on varDsc and liveness information must be recorded accordingly. LSRA seems not to has this information.
This reverts commit 1dec648.
BrianBohe
force-pushed
the
fixing_assert_on_variablelive_simd12
branch
from
69e6182 to
154ac7a
Compare
|
/azp run runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
The only difference is in coreclr_tests for the test case that was failing.
Here is the diff which removes the extra store on stack.
|
|
Errors are CI infrastructure related (timeouts) or unrelated |
ghost
locked as resolved and limited conversation to collaborators
Intend to fix 78898.
Op GT_STORE_LCL_VAR for target type SIMD12 always stores the result on the stack when FEATURE_SIMD is on. Liveness and the variable descriptor were updated after emitting this operation. This PR updates the tree's variable descriptor and liveness.
Having said that, LSRA assigned a target register for GT_STORE_LCL_VAR is assigned a target register in LSRA although the result (v01 arg0) is stored on the stack. For that reason, the tree target reg is being updated inside our specific case. LSRA already has the information about the available registers, then might be better to add this SIMD12 information there and avoid changing the tree after. I am opening this PR to see if this is triggering other problems.
The same behavior may be happening for GT_STOREIND for the same target type but I cannot get an example.