-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Extract fgMorph(Init/Copy)Block into their own classes. #53983
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
Conversation
|
/azp run runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
d8013f3 to
d2f27c5
Compare
|
/azp run runtime-coreclr jitstress, runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 2 pipeline(s). |
75803b5 to
b22c071
Compare
|
/azp run runtime-coreclr jitstress, runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 2 pipeline(s). |
b22c071 to
89d5d4d
Compare
src/coreclr/jit/morph.cpp
Outdated
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.
The previous version was working wrong with StackAllocation enabled, for reference types getClassSize always returns TARGET_POINTER_SIZE that is not how many bytes it takes on the stack, to see how it was fixed search for compObjectStackAllocation()
src/coreclr/jit/morphblock.cpp
Outdated
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.
there was a potential bug with trees like ADD(CNST X, ADD CNST Y, ADDR(LCL_VAR Z)) when we were morphing an assignment to field by field using the local var. In the past, we were ignoring X+Y offset for it. Lucky for us we don't have such cases.
src/coreclr/jit/morphblock.cpp
Outdated
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.
I am planning to delete fgMorphOneAsgBlockOp later so it is why I did not bother to move it into this class.
src/coreclr/jit/morphblock.cpp
Outdated
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.
this is an important change, in the past for ASG(struct<8> notPromotedStruct, 0) if we did not transform it as OneAsg (for example, the struct has holes and a custom layout) we would set lvaSetVarDoNotEnregister. Now we don't, LSRA won't try to enregister it because it can't enregister structs, but if we enable JitEnregStructLocals=1 we will start to see the diffs that we wanted. #52802 was an important step to make this pessimization deletion possible.
|
/azp run runtime-coreclr jitstress, runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
PTAL @BruceForstall, I think this is ready for review, cc @dotnet/jit-contrib |
src/coreclr/jit/morphblock.cpp
Outdated
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.
Nit: if you like terse debugging macros, this can be written as: DBEXEC(m_result != m_asg, m_result->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED) (would allow to use JITDUMP & DISPTREE below).
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.
Not a fan of it, will leave as if.
src/coreclr/jit/morphblock.cpp
Outdated
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.
Noting that this is an interesting assert in that it is actually testing that the constant node itself is well-formed. The implicit contract of CNS_INT of TYP_INT on 64 bit hosts is that the int32_t must be sign-extended to int64_t, which means that this assert will fail if something (say buggy GT_CAST folding...) creates dynSize with gtIconValue that is actually equal to UINT32_MAX (i. e. has upper bits set to zero), even as MorphDynamicBlock itself can work with such a constant (it won't be actually truncating anything).
src/coreclr/jit/morphblock.cpp
Outdated
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.
Just a note: would be nice to update these to the new dump format someday (which is much better in my opinion).
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.
Thanks, I will change this method further and include it. For now, I need to do the biggest code movements, which we will merge mostly based on the fact that there are no diffs rather than checking each small code piece that was moved.
After that these methods would become available for more changes.
|
/azp run runtime-coreclr jitstress, runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 2 pipeline(s). |
BruceForstall
left a comment
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.
LGTM. Looks like a nice cleanup, and framework for future work.
This is complicated enough that it seems like it would be useful to have a nice, long, file header comment describing the problem, what the code does, with some simple (and maybe even not so simple) examples. Like a design document/spec. That's not necessary for this change, but would be useful eventually to help people who need to work on the code in the future.
5b55600 to
94305c3
Compare
The main goal of this change is to simplify
fgMorph(Init/Copy)Blocklogic for future changes and current support.I am not expecting anybody to read through all code movements so suggest you read through the changes outside of these functions and ready the new code to see if it is easy to follow and if something is unclear. This change is no-asm diff (a few preparation PRs were merged to achieve that).
The changes outside of the 2 target functions:
LHSof an assignment is marked asGTF_DONT_CSEand there were a few places where we were missing it, they were fixed.IsLocalAddrExprgot a header and an additional argument, now for tree likeADD(LCL_VAR V01+16, 10)we would recover10as the offset, in the past were recording the sum fldSeq but not the offset.