-
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
Delete JitDoOldStructRetyping
artifacts.
#51092
Conversation
/azp run runtime-coreclr jitstress |
Azure Pipelines successfully started running 1 pipeline(s). |
PTAL @dotnet/jit-contrib |
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.
Nice to see all that code going away.
call->gtReturnType = returnType; | ||
} | ||
assert(returnType == TYP_STRUCT); | ||
assert((howToReturnStruct == SPK_ByValueAsHfa) || (howToReturnStruct == SPK_ByValue)); |
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 is a possible change in the functionality of this method before we were handling SPK_EnclosingType, now we don't.
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.
SPK_EnclosingType becomes unreachable without compDoOldStructRetyping
, I have added an assert that only SPK_ByValueAsHfa
and SPK_ByValue
can come here.
We used SPK_EnclosingType
only for structs that are smaller than TARGET_POINTER_SIZE and we were retyping them as int/long, now we keep them as structs, so now need for this part.
The part that is left handles the multi-reg case.
{ | ||
GenTree* op1 = op->AsObj()->Addr(); | ||
|
||
// We will fold away OBJ/ADDR, except for OBJ/ADDR/INDEX |
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.
Your change here also removes this folding,
This doesn't seem to be directly related to compDoOldStructRetyping
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.
It is also unreachable, the previous condition was:
if (!compDoOldStructRetyping() && (!op->IsCall() || !op->AsCall()->TreatAsHasRetBufArg(this)))
now it is
if (!op->IsCall() || !op->AsCall()->TreatAsHasRetBufArg(this))
so op can't be anything but call.
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.
Is the SPK_EnclosingType clause in fgUpdateInlineReturnExpressionPlaceHolder also now unreachable? Would be nice if it was.
I pushed the change to delete the handling, there are no asm diffs but Jit log is smaller now.
Excellent. Thanks.
Is the runtime/src/coreclr/jit/fginline.cpp Lines 620 to 666 in c5d16e0
|
It is reachable but we don't need to do anything for it, runtime/src/coreclr/jit/fginline.cpp Lines 577 to 584 in f33c440
we don't do retyping so there is no deferred work nowadays. I pushed the change to delete the handling, there are no asm diffs but Jit log is smaller now. For example, for
on unix x64 we have:
instead of
|
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.
Looks Good
Delete the code that was handling
JitDoOldStructRetyping = true
.