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

Support PUTARG_SPLIT(STRUCT LCL_VAR/LCL_FLD) on ARM/64 #70861

Merged

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Jun 16, 2022

Same as #70256 - we are enabling more TYP_STRUCT local nodes, now for PUTARG_SPLIT nodes.

We are expecting improvements in all but a series of test methods where we will have large regressions due to the use of a reserved register in a PUTARG_SPLIT of a very large struct: struct<MCCTest.VTypeD, 4608>. I think such scenario is rather unrealistic, but if there is agreement that we should nevertheless avoid this regression, I will make the changes necessary.

Diffs.

TODO: figure out what (if anything) to do with the regressions in test methods with large frames.
@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 Jun 16, 2022
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 16, 2022
@ghost
Copy link

ghost commented Jun 16, 2022

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

Issue Details

Same as #70256 - we are enabling more TYP_STRUCT local nodes, now for PUTARG_SPLIT nodes.

We are expecting improvements in all but a series of test methods where we will have large regressions due to the use of a reserved register in a PUTARG_SPLIT of a very large struct: struct<MCCTest.VTypeD, 4608>. I think such scenario is rather unrealistic, but if there is agreement that we should nevertheless avoid this regression, I will make the changes necessary.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@SingleAccretion SingleAccretion marked this pull request as ready for review June 17, 2022 09:20
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@jakobbotsch
Copy link
Member

I think such scenario is rather unrealistic, but if there is agreement that we should nevertheless avoid this regression, I will make the changes necessary.

Can you say a little more about what happens in these test functions to make the regression so large? What would the changes look like?

@SingleAccretion
Copy link
Contributor Author

Can you say a little more about what happens in these test functions to make the regression so large? What would the changes look like?

It is a combination of large frame + large structs.

Large frame makes us use the reserved register combined with adds to form the address for the source local; large structs make us do that a lot.

A fix I had in mind is to reserve an internal register when we have a large frame and use that instead of "plain" emitIns_R_Ss.

@jakobbotsch
Copy link
Member

Seems ok to me to avoid complicating the logic more, I don't think need to optimize for passing such a large struct by value.

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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants