JIT: Expand simple stfld addresses early#125141
JIT: Expand simple stfld addresses early#125141jakobbotsch wants to merge 3 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR updates CoreCLR JIT import/morph logic to preserve IL evaluation order for stfld when the field address involves a large offset (which may require an explicit null check), and adds a regression test to cover the scenario.
Changes:
- Add a new JIT regression test (
Runtime_125124) validating that RHS evaluation is not reordered past a null check for large-offsetstfld. - Teach the importer to expand “simple” instance field store addresses early (when the offset is not “big”), avoiding the need for an explicit null check in those cases.
- Add
impCanReorderWithNullCheckhelper and adjustString.get_Lengthintrinsic import by removing a redundantGTF_EXCEPTannotation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/JIT/Regression/Regression_ro_1.csproj | Adds the new regression test source file to the test project. |
| src/tests/JIT/Regression/JitBlue/Runtime_125124/Runtime_125124.cs | New regression test covering stfld RHS vs null-check ordering for large offsets. |
| src/coreclr/jit/lclmorph.cpp | Updates local address recognition to account for GT_LCL_ADDR offsets during struct field promotion/address morphing. |
| src/coreclr/jit/importercalls.cpp | Removes redundant manual GTF_EXCEPT marking for String.get_Length (array-length node already annotates exceptions). |
| src/coreclr/jit/importer.cpp | Adds reorderability helper and implements early expansion of small-offset stfld addresses with updated spilling logic. |
| src/coreclr/jit/compiler.h | Declares the new impCanReorderWithNullCheck helper. |
| // | ||
| bool Compiler::impCanReorderWithNullCheck(GenTree* tree) | ||
| { | ||
| if ((tree->gtFlags & GTF_PERSISTENT_SIDE_EFFECTS) != 0) |
There was a problem hiding this comment.
impCanReorderWithNullCheck only rejects GTF_PERSISTENT_SIDE_EFFECTS (ASG/CALL), but trees with GTF_ORDER_SIDEEFF (e.g., GT_CATCH_ARG) also cannot be legally reordered. Consider also checking GTF_ORDER_SIDEEFF here (or using a broader side-effect mask) so stfld spilling decisions remain correct in handlers and other ordering-sensitive cases.
| if ((tree->gtFlags & GTF_PERSISTENT_SIDE_EFFECTS) != 0) | |
| if ((tree->gtFlags & (GTF_PERSISTENT_SIDE_EFFECTS | GTF_ORDER_SIDEEFF)) != 0) |
| // True if it would be unobservable whether a null check threw or before the | ||
| // specified node. |
There was a problem hiding this comment.
The Return Value comment reads awkwardly: "...whether a null check threw or before the specified node." Consider rewording to clearly express the intended ordering (e.g., "threw before or after" / "was performed before or after").
| // True if it would be unobservable whether a null check threw or before the | |
| // specified node. | |
| // True if it would be unobservable whether a null check threw before or after | |
| // the specified node. |
| if (expandAddrInline) | ||
| { | ||
| op1->AsFieldAddr()->gtFieldLookup = fieldInfo.fieldLookup; | ||
| if (obj->IsLclVarAddr()) |
There was a problem hiding this comment.
lvFieldAccessed is only set when obj->IsLclVarAddr() (i.e., GT_LCL_ADDR with offset 0). If obj is a GT_LCL_ADDR with a non-zero offset (address of an interior field of a struct local), this is still a field access that can inform struct promotion decisions. Consider setting lvFieldAccessed for any GT_LCL_ADDR (or otherwise ensuring interior-address cases are covered) to avoid inconsistent promotion behavior.
| if (obj->IsLclVarAddr()) | |
| if (obj->OperIs(GT_LCL_ADDR)) |
|
|
||
| [Fact] | ||
| public static void TestEntryPoint() | ||
| { |
There was a problem hiding this comment.
The test relies on the initial value of the static flag s_barCalled. To make the test self-contained (and resilient to potential re-runs within the same process), consider explicitly resetting s_barCalled to false at the start of TestEntryPoint before executing the scenario.
| { | |
| { | |
| s_barCalled = false; |
Alternative to #125126