-
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
Remove GT_PUTARG_TYPE #68748
Remove GT_PUTARG_TYPE #68748
Conversation
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsThis change removes Diffs are from the following:
There are a lot of follow-up cleanups to be done, in particular it is now no longer necessary to have morph obtain the class handle from the arg nodes which should simplify some of that code. But I will leave that for a future change. Based on #68736
|
Keep the old CallArgABIInfo::ArgType for now since there are some non-trivial changes to be made to remove/reuse it.
/azp run runtime-coreclr jitstress, runtime-coreclr outerloop, runtime-coreclr libraries-jitstress, Fuzzlyn |
Azure Pipelines successfully started running 4 pipeline(s). |
Fuzzlyn issues are preexisting. cc @dotnet/jit-contrib PTAL @AndyAyersMS -- this should be ready for review. |
This is a zero-diff change against current main with forward sub disabled and the following patches applied: |
I would like to see us head this way over time—that is, remove most/all direct class handle references from the jit—so we can represent things like boxed value types that won't have class handles. |
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 this get removed.
// non-generic function, in which case we might see the __Canon in | ||
// the parameter type but exact types in the signature type. | ||
// | ||
// TODO-ARGS: Remove this quirk; we should be able to use the |
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.
Wasn't clear to me exactly what "this quirk" is referring to.
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 probably shouldn't call it a quirk, but we currently store the argument's class handle instead of the signature's class handle in CallArg::m_signatureClsHnd
. They are different in some cases, e.g. when inlining shared code where the inlining function has exact types. This todo is about storing the actual signature type (returned by the EE) instead of the argument's type. It's probably one of the first follow-ups I'll have after this PR so I'll avoid churning CR to clarify (unless there's other feedback).
This change removes
GT_PUTARG_TYPE
by consistently storing the signature type of the argument insideCallArg
right away when arguments are added, instead of only from morph and on.Diffs are from the following:
GT_PUTARG_TYPE
means we have fewer nodes.GT_PUTARG_TYPE
nodes, in particular when the inliner substituted a single-use local directly for the argument. This would introduce some unnecessary casts and no longer happens.gtFoldExpr
for args to calls that are inline candidates, even if we back out of the inline. Previously this would happen in some cases (due to bashing) and in some cases not (because we did not write the folded node pointer back to the CallArgs list)There are a lot of follow-up cleanups to be done, in particular it is now no longer necessary to have morph obtain the class handle from the arg nodes which should simplify some of that code. But I will leave that for a future change.
Based on #68736