Fix spill check for struct lclVars #23570
Conversation
|
/azp run coreclr-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@dotnet/jit-contrib PTAL |
With the 1st class struct changes, the `SPILL_APPEND` check for the case of an assignment to a lclVar needs to handle block ops as well as lclVar lhs. Fix #23545
|
I ran PMI diffs on x64, and there are zero diffs. I'm running x86 diffs now. |
|
Zero x86 diffs as well |
|
@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test |
|
@dotnet/dnceng - I'm getting "download failure" errors on the "Test pri0 Linux arm64 checked" leg, on several retries (e.g. 88d45f1d-2938-4d52-b7ac-a2f18683e194). And along with the failure, the "Publish logs" job gives "Artifact testbuild_Linux_arm64_checked_innerloop_Logs already exists for build 139295." |
|
@dotnet/jit-contrib ping - I'd really like to get Linux arm64 testing on this, but so far I have no reason to believe it will fail. Can I get a review of this? |
src/jit/importer.cpp
Outdated
| } | ||
| else if (lhs->OperIsBlk()) | ||
| { | ||
| lclVar = lhs->AsBlk()->Addr()->IsLocalAddrExpr(); |
There was a problem hiding this comment.
Is IsLocalAddrExpr really the right function to use here? There are a few that attempt to detect local addresses and they differ in the trees that they recognize. This one recognizes something like ADD(ADDR(LCL_VAR, 2) but that's less common during import. There's also impIsAddressInLocal that does something similar but instead of ADD nodes it looks for FIELD nodes, which of course are more common during import.
There was a problem hiding this comment.
Yes, I think you're right. IsLocalAddrExpr() would be the right thing to call after morph, but at this point we might see a FIELD. I'm going to try to get a repro case for that.
There was a problem hiding this comment.
Having spent some time on this, I'm fairly certain that this only needs to handle full assignments. The FIELD case is handled here: https://github.com/dotnet/coreclr/blob/master/src/jit/importer.cpp#L14507, and explicit stloc cases here: https://github.com/dotnet/coreclr/blob/master/src/jit/importer.cpp#L11103
.In the failure case the "assignment" is an initobj. The block cases handle this with a goto SPILL_APPEND which lands here. Pretty much anything else that would modify a local would involve explicitly taking its address (not just in the context of doing a field store).
I gave some thought to whether an assert could be added, but I was unable to do so. So, I'm actually going to simplify this check.
| // This is done for non-block assignments in the handling of stloc. | ||
| if ((op1->OperGet() == GT_ASG) && varTypeIsStruct(op1->gtOp.gtOp1) && | ||
| (op1->gtOp.gtOp1->gtOper == GT_LCL_VAR)) | ||
| if ((op1->OperGet() == GT_ASG) && varTypeIsStruct(op1->gtGetOp1())) |
There was a problem hiding this comment.
Would be nice to also describe why we need to spill in the first place.
|
I added a comment to the offending PR (https://github.com/dotnet/coreclr/pull/22255/files#diff-edc46b80f57431489a82311948a8234d) that shows where this case was previously being handled. |
…t usings from test
|
@dotnet-bot test Windows_NT x64 Checked CoreFX Tests |
|
@dotnet/dnceng - another isntance of the "Test pri0 Linux arm checked" leg being cancelled. |
|
I can't figure out how to retry that leg. I'm going to merge this without that leg. |
The assert introduced in dotnet#23570 was overly aggressive, and didn't account for the fact that pointer arithmetic can exist in the IL, not just introduced by morph. Fix #23693
The assert introduced in #23570 was overly aggressive, and didn't account for the fact that pointer arithmetic can exist in the IL, not just introduced by morph. Fix #23693
* Fix spill check for struct lclVars With the 1st class struct changes, the `SPILL_APPEND` check for the case of an assignment to a lclVar needs to handle block ops as well as lclVar lhs. Fix dotnet/coreclr#23545 Commit migrated from dotnet/coreclr@f036d23
The assert introduced in dotnet/coreclr#23570 was overly aggressive, and didn't account for the fact that pointer arithmetic can exist in the IL, not just introduced by morph. Fix dotnet/coreclr#23693 Commit migrated from dotnet/coreclr@abec165
With the 1st class struct changes, the
SPILL_APPENDcheck for the case of an assignment to a lclVar needs to handle block ops as well as lclVar lhs.Fix #23545