JIT: Add ordering side effects in fgMorphExpandInstanceField #92998
Conversation
In dotnet#92710 I removed an ordering side effect set on a LCL_VAR node in fgMorphExpandInstanceField, since the side effect is meaningless on a local. However, I did not realize that setting the ordering effect was important because propagating it to parent nodes was actually necessary. This PR explicitly sets the ordering effect on the ADD<byref> nodes created, since forming these and reporting them to GC is illegal if the base is invalid. Additionally, it sets the ordering effect on the consuming indirection; if that indirection made use of the fact that the FIELD_ADDR is non-null to become non-faulting, then it has an ordering dependency once we expand the FIELD_ADDR to a control-flow based null-check. This is not actually necessary since we always set end up with an ADD<byref> that propagates it, but that seems more like a coincidence than anything else. Fix dotnet#92990
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsIn #92710 I removed an ordering side effect set on a LCL_VAR node in fgMorphExpandInstanceField, since the side effect is meaningless on a local. However, I did not realize that setting the ordering effect was important because propagating it to parent nodes was actually necessary. This PR explicitly sets the ordering effect on the ADD nodes created, since forming these and reporting them to GC is illegal if the base is invalid. Additionally, it sets the ordering effect on the consuming indirection; if that indirection made use of the fact that the FIELD_ADDR is non-null to become non-faulting, then it has an ordering dependency once we expand the FIELD_ADDR to a control-flow based null-check. This is not actually necessary since we always set end up with an ADD that propagates it, but that seems more like a coincidence than anything else. Fix #92990
|
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
cc @dotnet/jit-contrib PTAL @BruceForstall |
src/coreclr/jit/compiler.h
Outdated
| GenTreeIndir* Consumer = nullptr; // Indirection using this address. | ||
| size_t TotalOffset = 0; // Sum of offsets between the top-level indirection and here (current context). |
There was a problem hiding this comment.
nit: why change the naming style? We aren't very consistent with https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/clr-jit-coding-conventions.md#8.7, but we are pretty consistent to not capitalize local/field names.
There was a problem hiding this comment.
I reverted this... mainly I think the m_ prefix on fields meant to be used publicly looks off (I like the convention, but only for private fields, and I thought that's only where it applied).
In #92710 I removed an ordering side effect set on a LCL_VAR node in fgMorphExpandInstanceField, since the side effect is meaningless on a local. However, I did not realize that setting the ordering effect was important because propagating it to parent nodes was actually necessary.
This PR explicitly sets the ordering effect on the ADD nodes created, since forming these and reporting them to GC is illegal if the base is invalid.
Additionally, it sets the ordering effect on the consuming indirection; if that indirection made use of the fact that the FIELD_ADDR is non-null to become non-faulting, then it has an ordering dependency once we expand the FIELD_ADDR to a control-flow based null-check. This is not actually necessary since we always set end up with an ADD that propagates it, but that seems more like a coincidence than anything else.
Fix #92990