JIT: Spill stfld data if nullcheck on object interferes#125126
JIT: Spill stfld data if nullcheck on object interferes#125126jakobbotsch wants to merge 2 commits intodotnet:mainfrom
Conversation
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR addresses a JIT ordering issue where the explicit nullcheck introduced for large instance field offsets can interfere with the required IL evaluation order for stfld, potentially causing the store’s RHS to be evaluated after the nullcheck (and therefore not executed if the nullcheck throws). A new regression test is added to validate the intended ordering.
Changes:
- Add a JIT regression test ensuring RHS evaluation is not skipped when a large-offset
stfldthrowsNullReferenceException. - Update the importer to spill the
stfldvalue when it cannot be safely reordered with the nullcheck. - Remove an explicit
GTF_EXCEPTflag assignment forString.Lengthintrinsic, relying on standard indirection exception annotation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/JIT/Regression/Regression_ro_1.csproj | Includes the new regression test source file in the test project. |
| src/tests/JIT/Regression/JitBlue/Runtime_125124/Runtime_125124.cs | Adds a regression test covering stfld RHS ordering vs. explicit nullcheck insertion for large offsets. |
| src/coreclr/jit/importercalls.cpp | Removes a manual GTF_EXCEPT flag set for String.get_Length intrinsic. |
| src/coreclr/jit/importer.cpp | Adds impCanReorderWithNullCheck and spills stfld data when ordering could be violated by a nullcheck. |
| src/coreclr/jit/compiler.h | Declares impCanReorderWithNullCheck. |
| if ((tree->gtFlags & GTF_PERSISTENT_SIDE_EFFECTS) != 0) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| if (((tree->gtFlags & GTF_EXCEPT) != 0) && (gtCollectExceptions(tree) != ExceptionSetFlags::NullReferenceException)) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
impCanReorderWithNullCheck currently ignores GTF_ORDER_SIDEEFF. If the stfld value has an ordering side effect (e.g., a volatile indirection sets GTF_ORDER_SIDEEFF), allowing it to be reordered with the explicit nullcheck introduced for large field offsets can violate IL evaluation order by skipping the volatile operation when the nullcheck throws. Consider returning false when tree->gtFlags includes GTF_ORDER_SIDEEFF (and possibly other non-persistent ordering-only effects) so the value is spilled in these cases.
No description provided.