-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
struct improvement, part1: create more LCL_FLD #48377
struct improvement, part1: create more LCL_FLD #48377
Conversation
/azp run runtime-coreclr jitstress |
Azure Pipelines successfully started running 1 pipeline(s). |
a48f4f3
to
f17ef0e
Compare
PTAL @dotnet/jit-contrib, this is the first part of struct improvement for 6.0. It is known that such optimizations often reveal issues in VN/CSE phases so I am writing tests to add before the merge, please tell me if you know any particular transformations that could be affected by this. |
/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs |
Azure Pipelines successfully started running 2 pipeline(s). |
f17ef0e
to
d9e0e2c
Compare
/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs |
Azure Pipelines successfully started running 2 pipeline(s). |
d9e0e2c
to
4159742
Compare
/azp run runtime-coreclr jitstress, runtime-coreclr jitstressregs |
Azure Pipelines successfully started running 2 pipeline(s). |
The PR has been updated and now it passes all tests, + I am adding a new test here with problematic struct copies + #49504 to improve VN checks. The updated diffs, framework libraries:
Benchmark diffs:
The regressions are from:
PTAL @dotnet/jit-contrib |
// for calls that return multiple registers | ||
// Don't use field by field assignment if the src is a call, | ||
// lowering will handle it without spilling the call result into memory | ||
// to access the individual fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two questions:
-
What is the behavior when assigning a call result to a promoted struct that has a single field.
-
How does lower patch things up?
Don't we create a copy block here, which makes the dest become address taken?
How can lower fix that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- What is the behavior when assigning a call result to a promoted struct that has a single field.
This is a special case covered by CanBeReplacedWithItsField
, it checks exactly for promoted struct with 1 field without holes, in this case the dst is not address taken. it happens in LowerStoreLocCommon
:
runtime/src/coreclr/jit/lower.cpp
Line 3119 in 54d2b35
JITDUMP("Replacing an independently promoted local var V%02u with its only field V%02u for the store " |
- How does lower patch things up?
for the case of ASG(LCL_VAR, call)
it would come as STORE_LCL_VAR(call)
to LowerStoreSingleRegCallStruct
and it will generate STORE_IND
or STORE_BLK
. Store_blk is used when the struct has the size of 3, 5, 6, 7 bytes. The details are in LowerCallStruct
.
runtime/src/coreclr/jit/lower.cpp
Line 3543 in 54d2b35
void Lowering::LowerStoreSingleRegCallStruct(GenTreeBlk* store) |
ping @dotnet/jit-contrib I recommend reviewing it with "Hide whitespace changes checked". The idea is quite simple when we have
we want to generate a field-by-field assignment for V01. In order to do that we need to access
when we can actually do
instead, it prevents |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look ok to me.
Brian was recently in this code so should also approve.
fgMorphCopyBlock
is complicated and it is hard to have confidence that what it is doing is correct. You might think about how to rework it to make what it is doing more obvious.
It is on my plate but pretty far, with struct enregistration this transformation needs to be moved from morph into a later phase, I have a description of my design in PostponeMorphingOfBlockOperations.md. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a few notes
fgMorphCopyBlock
has become an unreadable beast...
When we copy a promoted struct to an unpromoted struct use
LCL_FLD unpromoted fldOffset
instead ofIND(ADDR(LCL_VAR unpromoted, fldOffset)
, it prevents creating additional instructions, including taking address if `unpromoted struct'.So it deletes marking 'unpromoted struct' as address is taken and allows more optimizations with it.
Edit: see the updated diffs in the comment below, the PR was rewritten to use
LCL_FLD
only whenFIELD_HANDLE
belongs to LCL_VAR's struct type.