This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Preserve Vector Arg registers on Arm64 #22257
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is this fixing any actual functional bug; or is this a prep-work/cleanup for other changes? |
|
@jkotas, there isn't an actual bug (that I know of) that this fixes (other than the #14371). Since we support the 128-bit vector types |
jashook
requested review from
jashook,
janvorli,
adityamandaleeka and
BruceForstall
jashook
approved these changes
Jan 29, 2019
src/vm/callingconvention.h
Outdated
| @@ -573,8 +573,8 @@ class ArgIteratorTemplate : public ARGITERATOR_BASE | |||
|
|
|||
| if (TransitionBlock::IsFloatArgumentRegisterOffset(argOffset)) | |||
| { | |||
| // Dividing by 8 as size of each register in FloatArgumentRegisters is 8 bytes. | |||
| pLoc->m_idxFloatReg = (argOffset - TransitionBlock::GetOffsetOfFloatArgumentRegisters()) / 8; | |||
| // Dividing by 8 as size of each register in FloatArgumentRegisters is 16 bytes. | |||
There was a problem hiding this comment.
nit: Dividing by 8 as size of each register in FloatArgumentRegisters is 16 bytes should be Dividing by 16 as size of each register in FloatArgumentRegisters is 16 bytes
|
I think it is worth running some of the outerloop arm64 jobs on this pr before merging |
|
@jashook - Yes, I am planning to do so (and suggestions are welcome!), but I wanted to get some review first, as I'm not familiar with most of this code, and thought perhaps one of the reviewers might catch something I've missed. |
|
@BruceForstall FYI we were discussing this yesterday |
|
@dotnet-bot help |
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
Welcome to the dotnet/coreclr Perf help The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked jitstress2_jitstressregs1 Build and Test |
janvorli
reviewed
Jan 29, 2019
src/vm/arm64/asmhelpers.S
Outdated
| @@ -121,18 +121,18 @@ LEAF_END HelperMethodFrameRestoreState, _TEXT | |||
| // The call in ndirect import precode points to this function. | |||
| NESTED_ENTRY NDirectImportThunk, _TEXT, NoHandler | |||
|
|
|||
| PROLOG_SAVE_REG_PAIR_INDEXED fp, lr, -160 | |||
| PROLOG_SAVE_REG_PAIR_INDEXED fp, lr, -240 | |||
There was a problem hiding this comment.
@CarolEidt I am trying to figure out why the frame size has changed to 240. Argument registers occupy sp+16.. sp+87, then you have added 8 bytes of padding to align the float registers address to 16 bytes and then you have 128 bytes of the floating point arg registers. 96+128=224, but you have 240 there.
There was a problem hiding this comment.
That's a good question, and I can't recall the reason. It may be an arithmetic error, but I made those changes in my first round of working on that, and it's been awhile so I don't recall (this is why I try to do a better job of writing comments!). Let me look into this.
There was a problem hiding this comment.
I think this was just a mistake. I've changed it to 224.
They are not special interop types today. They are just regular structs. Do we plan to do the work to pass |
Yes, that's the plan. |
|
What is the issue tracking that work? |
The specific HVA issue is #16022. There is an overarching issue #20427 |
src/vm/argdestination.h
Outdated
| memcpyNoGCRefs(dest, src, fieldBytes); | ||
| // Copy 4 or 8 bytes from src. | ||
| UINT64 val = typeFloat ? *((UINT32*)src + i) : *((UINT64*)src + i); | ||
| // Alyways store 8 bytes |
|
test OSX10.12 x64 Checked Innerloop Build and Test |
Fix #14371
CarolEidt
force-pushed
the
PreserveArm64Vectors
branch
from
65b7afd
to
a113ef1
Compare
CarolEidt
changed the title
|
I believe I've addressed all of the feedback. Could I get another review? |
|
@jashook - do you have recommendations for outer-loop arm64 testing? |
|
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked jitstress1 Build and Test |
janvorli
reviewed
Jan 31, 2019
src/vm/arm64/asmhelpers.asm
Outdated
| @@ -184,18 +184,18 @@ Done | |||
| ; The call in ndirect import precode points to this function. | |||
| NESTED_ENTRY NDirectImportThunk | |||
|
|
|||
| PROLOG_SAVE_REG_PAIR fp, lr, #-160! | |||
| PROLOG_SAVE_REG_PAIR fp, lr, #-240! | |||
There was a problem hiding this comment.
You've forgotten to change the 240->224 in this file too.
There was a problem hiding this comment.
@janvorli - thanks! I couldn't figure this out at first because I had "Asmhelpers.asm" open in VS, and it was showing the 224 offset - but it was the one under the obj directory, so changing that one was not useful :-(
Fixed now.
|
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked jitstress1 Build and Test |
|
@jashook - I can't figure out how to trigger the stress jobs - can you advise? |
|
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked r2r_jitstressregs1 Build and Test |
|
Well that is weird. |
|
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked jitstress1 Build and Test |
|
@mmitche why would the trigger phrase be ignored for Carol? |
|
No reason...maybe it just ignored her message altogether? |
BruceForstall
approved these changes
Jan 31, 2019
|
Infrastructure failures |
|
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked jitstress1 Build and Test |
1 similar comment
|
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked jitstress1 Build and Test |
|
@dotnet-bot test Windows_NT arm64 Cross Checked jitstress2_jitstressregs1 Build and Test |
|
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked jitstress1 Build and Test |
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
* Preserve Vector Arg registers on Arm64 Fix dotnet/coreclr#14371 Commit migrated from dotnet/coreclr@f10d5be
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #14371