Fix enregistered lclFld bug#18418
Conversation
|
The reason this was not encountered previously is that, prior to #17996, the |
In `impFixupStructReturnType()`, don't transform to `GT_LCL_FLD` if we have a scalar lclVar. Also, to avoid future bad codegen, add verification and recovery code to Lowering. Fix #18408
|
Is this something we could catch earlier with some kind of IR check? |
Possibly; I thought it best to add a check just prior to where it matters - i.e. in |
…so it can be called from the assert in Lowering.
|
@dotnet/jit-contrib PTAL |
| return false; | ||
| } | ||
|
|
||
| // Don't enregister if the ref count is zero. |
There was a problem hiding this comment.
This section of code, and the above check for !enregisterLocalVars were extracted from identifyCandidates() so that isRegCandidate can be called from the assert in Lowering.
| op->ChangeOper(GT_LCL_FLD); | ||
| // It is possible that we now have a lclVar of scalar type. | ||
| // If so, don't transform it to GT_LCL_FLD. | ||
| if (varTypeIsStruct(lvaTable[op->AsLclVarCommon()->gtLclNum].lvType)) |
There was a problem hiding this comment.
Could be AsLclVar but I guess it doesn't really matter.
There was a problem hiding this comment.
Yes, I think I'm programmed to use AsLclVarCommon
| // Any tracked lclVar accessed by a LCL_FLD or STORE_LCL_FLD should be marked doNotEnregister. | ||
| // This method checks, and asserts in the DEBUG case if it is not so marked, | ||
| // but in the non-DEBUG case (asserts disabled) set the flag so that we don't generate bad code. | ||
| // Thie ensures that the local's value is valid on-stack as expected for a *LCL_FLD. |
| #if CPU_HAS_FP_SUPPORT | ||
| case TYP_FLOAT: | ||
| case TYP_DOUBLE: | ||
| if (compiler->opts.compDbgCode) |
There was a problem hiding this comment.
I think the whole switch would be more readable if this and other similar cases below are changed to return !compiler->opts.compDbgCode;.
| { | ||
| return false; | ||
| } | ||
| break; |
|
@dotnet/jit-contrib ping - I'd like to get this merged to unblock corert moving to the latest |
This reverts commit 82134a0.
* Fix enregistered lclFld bug In `impFixupStructReturnType()`, don't transform to `GT_LCL_FLD` if we have a scalar lclVar. Also, to avoid future bad codegen, add verification and recovery code to Lowering. Fix dotnet/coreclr#18408 * Extract the full conditions for whether a lclVar is a reg candidate, so it can be called from the assert in Lowering. * Review feedback Commit migrated from dotnet/coreclr@82134a0
4 participants
In
impFixupStructReturnType(), don't transform toGT_LCL_FLDif we have a scalar lclVar.Also, to avoid future bad codegen, add verification and recovery code to Lowering.
Fix #18408