Forbid creating layout-less TYP_STRUCT LCL_FLD nodes
#70170
Conversation
ghost
added
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsThis is essentially an improved version of the reverted #66251, addressing the promotion issue that was seen in the subsequent test runs, and simplifying some code. We're expecting a tiny number of diffs. Sources:
|
SingleAccretion
force-pushed
the
No-Layout-Less-Struct-LclFld
branch
2 times, most recently
from
74858dc to
9a3bf94
Compare
|
@dotnet/jit-contrib I would like to request stress and libraries stress runs for this change. |
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
SingleAccretion
force-pushed
the
No-Layout-Less-Struct-LclFld
branch
from
9a3bf94 to
3e39c16
Compare
SingleAccretion
force-pushed
the
No-Layout-Less-Struct-LclFld
branch
from
3e39c16 to
13fd37a
Compare
|
@dotnet/jit-contrib |
src/coreclr/jit/morph.cpp
Outdated
| { | ||
| break; | ||
| } | ||
| assert(!gtIsActiveCSE_Candidate(op1)); // GT_ADDRs cannot be CSE candidates. |
There was a problem hiding this comment.
Immediate reason is that optIsCSEcandidate always returns false for them.
Reasons why we wouldn't want / be able to CSE them:
- CSEing
ADDRs that point to tracked locals would break things. - CSEing
ADDRs that point to address-exposed locals is likely not very useful because they're cheap to rematerialize (with the exception of non-XARCH platforms + huge frames, I suppose). - CSEing
ADDR(HWI/SIMD)would break things. - All other
ADDRs (ADDR(IND/BLK/OBJ)) are pretty much transient (get created and then immediately morphed away).
There was a problem hiding this comment.
I don't really see a good reason to bake an implicit assumption that we are never going to be able to CSE GT_ADDR nodes, e.g. in a case like: https://godbolt.org/z/fWEhhGaoK
There was a problem hiding this comment.
I don't think it is unreasonable to CSE local addresses, but I do not see us doing that with ADDRs, because they will increasingly only get used for tracked locals. Note: your example would have LCL_VAR_ADDR nodes.
There was a problem hiding this comment.
but I do not see us doing that with ADDRs, because they will increasingly only get used for tracked locals
Well, ideally that should not matter. The same example with a promoted struct shows the same codegen difference between Clang/GCC and RyuJIT.
There was a problem hiding this comment.
Well, ideally that should not matter
Do you mean not matter as in that we should be able to CSE even addresses of tracked locals? That seems like a hard problem.
Regardless, I will put the CSE check back in.
There was a problem hiding this comment.
Do you mean not matter as in that we should be able to CSE even addresses of tracked locals? That seems like a hard problem.
I agree, the bookkeeping required for this would surely make it infeasible/not worth it. I'd just hope this wasn't a global invariant (but given that you are way more familiar with the story around GT_ADDR and the other addressing nodes, maybe this hope is unfounded :) ).
|
Failure is #70549 |
3 participants
This is essentially an improved version of the reverted #66251, addressing the promotion issue that was seen in the subsequent test runs, and simplifying some code.
We're expecting a small number of diffs. Sources:
gtSetEvalOrderseesADDR(LCL_FLD byte)as a bit costlier thanADDR(LCL_FLD struct).BLK(ADDR(LCL_VAR) + OFFSET)is now simplified toBLK(ADDR(LCL_FLD)). This can be a size regression in case we would choose to use an inline copy sequence instead of a helper call.LCL_VARs and does not type checkLCL_FLDs.There is also a small TP regression that will be payed for with the next change and #69991.
Diffs.