Skip to content
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

Merged
merged 22 commits into from
May 9, 2022
Merged

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented May 1, 2022

This change removes GT_PUTARG_TYPE by consistently storing the signature type of the argument inside CallArg right away when arguments are added, instead of only from morph and on.

Diffs are from the following:

  • We sometimes do new forward subs now since no GT_PUTARG_TYPE means we have fewer nodes.
  • Previously, it was possible with nested inlining to introduce redundant 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.
  • We now consistently call 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

@ghost ghost assigned jakobbotsch May 1, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 1, 2022
@ghost
Copy link

ghost commented May 1, 2022

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

This change removes GT_PUTARG_TYPE by consistently storing the signature type/ABI type of the argument inside CallArgABIInformation right away when arguments are added, instead of only from morph and on.

Diffs are from the following:

  • We sometimes do new forward subs now since no GT_PUTARG_TYPE means we have fewer nodes.
  • Previously, it was possible with nested inlining to introduce redundant 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.
  • We now consistently call 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

Author: jakobbotsch
Assignees: jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr outerloop, runtime-coreclr libraries-jitstress, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@jakobbotsch
Copy link
Member Author

jakobbotsch commented May 8, 2022

Fuzzlyn issues are preexisting.
win-arm64 and win-arm failures are dotnet/arcade#9284.
jitstress linux-arm and linux-arm64 failures are #68690.
libraries-jitstress linux-arm64 failures look like #68513, #68756.

cc @dotnet/jit-contrib PTAL @AndyAyersMS -- this should be ready for review.
I did wonder if we should include a ClassLayout* instead of the signature class handle, potentially adding methods to make it useful for ABI purposes, but after looking at the code that will end up using it to begin with I just went with the class handle.

@jakobbotsch jakobbotsch marked this pull request as ready for review May 8, 2022 15:02
@jakobbotsch
Copy link
Member Author

This is a zero-diff change against current main with forward sub disabled and the following patches applied:
https://github.com/dotnet/runtime/compare/main...jakobbotsch:remove-GT_PUTARG_TYPE-zero-diff-target?expand=1

@AndyAyersMS
Copy link
Member

I did wonder if we should include a ClassLayout* instead of the signature class handle,

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.

Copy link
Member

@AndyAyersMS AndyAyersMS left a 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
Copy link
Member

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.

Copy link
Member Author

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).

@jakobbotsch jakobbotsch merged commit f8fa9f6 into dotnet:main May 9, 2022
@jakobbotsch jakobbotsch deleted the remove-GT_PUTARG_TYPE branch May 9, 2022 09:57
@ghost ghost locked as resolved and limited conversation to collaborators Jun 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants