Don't hoist IConHandle statics above cctors #11694
Conversation
|
@briansull PTAL Still running diffs... |
|
|
||
| GenTreePtr tlsRef = gtNewIconHandleNode(WIN32_TLS_SLOTS, GTF_ICON_TLS_HDL); | ||
|
|
||
| // Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS |
There was a problem hiding this comment.
Also line 6468 contains a dangerous SetOper call which can cause the
flag GTF_FLD_INITCLASS to be interpreted as GTF_IND_REFARR_LAYOUT
There was a problem hiding this comment.
6468 is dealing with instance fields (it's under the if (objRef) on line 6239 with else on line 6500)
| // truly dependent on the cctor. So a more precise approach would be to separately propagate | ||
| // isCctorDependent and isAddressWhoseDereferenceWouldBeCctorDependent, but we don't for simplicity/throughput; | ||
| // the constant itself would be considered non-hoistable anyway, since optIsCSEcandidate returns | ||
| // false for constants. |
There was a problem hiding this comment.
since optIsCSEcandidate returns false for constants.
This might not continue to be true in the future.
Is it possible to flag the indirection instead of the constant?
Or are we out of flag bits on GT_IND?
There was a problem hiding this comment.
Yes, GT_IND is already dipping into the common bits:
#define GTF_IND_NONFAULTING 0x00000800 // An indir that cannot fault. GTF_SET_FLAGS is not used on indirsWe could (maybe?) similarly "steal" GTF_ZSF_SET or GTF_UNSIGNED for a new GT_IND flag, though I think the extra conservativism may be warranted here... with this change as-is, we'll block hoisting static field addresses produced by ldsflda (if the field is marked CORINFO_FLG_FIELD_INITCLASS and we aren't hoisting the cctor call with it). The importer does the marking now, but the importer doesn't have the context to know when a GT_IND might be fed by a prior ldsflda, so it seems risky to leave the ldsflda addresses unmarked. Using two bools here to track cctor-dependence would let us keep the annotations on the address and so avoid the flag sharing and the ldsflda+indir problem, I just didn't think it was worth it with no/little current benefit. I could be persuaded to change it though, what do you think?
There was a problem hiding this comment.
I'm comfortable with this fix for now and don't want to complicate this right now by fishing for extra flag bits for the GT_IND node.
|
jit-diff reports no diffs, but crossgen takes different codepaths for statics than jitting; will check spmi diffs when back online |
| // of the cell that the runtime will fill in with the address | ||
| // of the static field; in both of those cases, the constant | ||
| // is what gets flagged. | ||
|
|
There was a problem hiding this comment.
This is likely to conflict with Bruce's GT Flags reformatting change
See #11700
There was a problem hiding this comment.
Yeah, I noticed that. I'll rebase/resolve if he merges first; I'd like to run desktop testing on this before merging.
|
@dotnet-bot test Windows_NT x64 CoreCLR Perf Tests Correctness test Windows_NT x86 CoreCLR Perf Tests Correctness |
These constants appear loop invariant, hence so do their dereferences when a loop has no memory side-effects. Add flag `GTF_ICON_INITCLASS` similar to `GTF_FLD_INITCLASS`/`GTF_CLS_VAR_INITCLASS`, and use it to prevent hoisting such references without also hoisting their static init helper calls. Resolves #11689
JosephTremoulet
force-pushed
the
IconHoist
branch
from
4827521 to
66e5e7f
Compare
The code in loop hoisting visiting a comma for a static init helper call and static field access tried to clear the childrenHoistable bit for the field, but had an off-by-one error and was instead clearing the bit for the helper call, which could lead to missed legal helper call hoists. Update that code to simply assert that the flag is already set for the field (using the correct index), which must be true since the check is guarded by a check that childIsCctorDependent is true for it.
|
Desktop testing showed that this code had an off-by-one error that was artificially blocking the hoisting of cctor calls in some cases. I've pushed a fix for that in this PR; @briansull PTAL. With that, SPMI reports diffs in just 34 unique methods. The ones I looked into were all attributable to an existing issue making the static var hoisting a bit more conservative than it needs to be, for which I've filed #11824. So I think this is ready to merge (and likely port to 2.0/4.7.1) if the last change looks agreeable. |
|
Go ahead and check this in |
|
Thanks! |
5 participants
These constants appear loop invariant, hence so do their dereferences
when a loop has no memory side-effects. Add flag
GTF_ICON_INITCLASSsimilar to
GTF_FLD_INITCLASS/GTF_CLS_VAR_INITCLASS, and use it toprevent hoisting such references without also hoisting their static
init helper calls.
Resolves #11689