Stop marking volatile indirections as NO_CSE
#62043
Conversation
ghost
added
the
community-contribution
dotnet-issue-labeler
bot
added
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsIt is unnecessary - we model volatile indirections as mutating the global heap in value numbering and thus they can only ever be CSE defs, which is legal in our model (only volatile operations serve as mutators as per ECMA). We get some good small diffs this way. However, the real purpose of this change is making fixing the incorrect assertions for L-value indirections (#13762, reproduction), which will be identified by the presence of the We could venture and add something like a
|
SingleAccretion
force-pushed
the
Incorrect-Null-Assertions
branch
from
efafc41 to
d1a7d8a
Compare
|
@dotnet/jit-contrib |
|
@dotnet/jit-contrib I am OOF next week so if somebody else wants to give this a lookover before I'm back then please do. |
It is unnecessary - we model volatile indirections as mutating the global heap in value numbering and thus they can only ever be CSE defs, which is legal in our model (only volatile operations serve as mutators as per ECMA). We get some good small diffs this way. However, the real purpose of this change is making fixing the incorrect assertions for L-value indirections, which will be identified by the presence of the GTF_NO_CSE flag, as that is the only reliable way to detect L-values in the compiler, mostly zero-diff. We could venture and add something like a GTF_LVALUE flag (I think we had one with that very name at some point...), but that seems very risky - how many a place today forgets about GTF_NO_CSE, certainly plumbing the new flag through will not be a pleasant, or good, long-term solution. The proper solution is to delete GT_ASG, but that, of course, is not something done in one weekend :), and it seems bad to leave known silent bad codegen bugs hanging around in the meantime.
SingleAccretion
force-pushed
the
Incorrect-Null-Assertions
branch
from
d1a7d8a to
10877fd
Compare
It is unnecessary - we model volatile indirections as mutating the global heap in value numbering and thus they can only ever be CSE defs, which is legal in our model (only volatile operations serve as mutators as per ECMA).
We get some good small diffs this way.
However, the real purpose of this change is making fixing the incorrect assertions for L-value indirections (#13762, reproduction), which will be identified by the presence of the
GTF_NO_CSEflag, as that is the only reliable way to detect L-values in the compiler, mostly zero-diff.We could venture and add something like a
GTF_LVALUEflag (I think we had one with that very name at some point...), but that seems very risky - how many a place today forgets aboutGTF_NO_CSE, certainly plumbing the new flag through will not be a pleasant, or good, long-term solution. The proper solution is to deleteGT_ASG, but that, of course, is not something done in one weekend :), and it seems bad to leave known silent bad codegen bugs hanging around in the meantime.Full diffs.