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

[NativeAOT] Save full ARM64 SIMD arg registers in UniversalTransition #74888

Merged
merged 2 commits into from
Sep 1, 2022

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Aug 31, 2022

On arm64 entire SIMD registers (q0-q7) could be used for argument passing.

I am not sure if JIT uses HVA argument passing for NativeAOT, maybe it is disabled, but it looks like our save/restore of arguments in UniversalTransition frames could be incomplete.

@VSadov
Copy link
Member Author

VSadov commented Aug 31, 2022

Re: PreStub in CoreClr.

It looks like PreStub on ARM64 stores full q registers

ThePreStub uses PROLOG_WITH_TRANSITION_BLOCK macro

NESTED_ENTRY ThePreStub

        PROLOG_WITH_TRANSITION_BLOCK
        ... 

PROLOG_WITH_TRANSITION_BLOCK uses SAVE_FLOAT_ARGUMENT_REGISTERS

        IF "$SaveFPArgs" != ""
__PWTB_SaveFPArgs SETL $SaveFPArgs
        ELSE
__PWTB_SaveFPArgs SETL {true}
        ENDIF

. . . 

        ; Spill argument registers.
        SAVE_ARGUMENT_REGISTERS        sp, __PWTB_ArgumentRegisters

        IF __PWTB_SaveFPArgs
        SAVE_FLOAT_ARGUMENT_REGISTERS  sp, __PWTB_FloatArgumentRegisters
        ENDIF

and SAVE_FLOAT_ARGUMENT_REGISTERS saves q registers

; Reserve 128 bytes of memory before calling SAVE_FLOAT_ARGUMENT_REGISTERS

@VSadov
Copy link
Member Author

VSadov commented Aug 31, 2022

Interestingly nativeaot has SAVE_FLOAT_ARGUMENT_REGISTERS defined on unix, but it could be dead code, I could not track it to its use.

// Reserve 64 bytes of memory before calling SAVE_FLOAT_ARGUMENT_REGISTERS

@VSadov
Copy link
Member Author

VSadov commented Aug 31, 2022

There is also use of d0, d1 in RhCallDescrWorker

It looks like this corresponds to CoreClr CallDescrWorkerInternal, which has an additional conditional handling of HVA returns

;;VectorHFAReturn return case

, and the float argument part deals with q registers (only d portions in nativeaot counterpart)

I do not know if nativeaot and CoreClr should match here

@jkotas
Copy link
Member

jkotas commented Aug 31, 2022

I do not know if nativeaot and CoreClr should match here

RhCallDescrWorker is probably a dead code.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is candidate for release/7.0 backport. The arm64 calling convention is passing HFA in these registers.

@VSadov
Copy link
Member Author

VSadov commented Sep 1, 2022

I will check once more on the nativeaot+unix version of SAVE_FLOAT_ARGUMENT_REGISTERS - to be sure it is unused.

@VSadov
Copy link
Member Author

VSadov commented Sep 1, 2022

I will check once more on the nativeaot+unix version of SAVE_FLOAT_ARGUMENT_REGISTERS - to be sure it is unused.

it is unused. I will just remove it to be less confusing, since it seems to be subtly incorrect.

@VSadov VSadov closed this Sep 1, 2022
@VSadov VSadov reopened this Sep 1, 2022
@VSadov
Copy link
Member Author

VSadov commented Sep 1, 2022

Tests are not starting. :-/

@VSadov
Copy link
Member Author

VSadov commented Sep 1, 2022

Although the new commit just deletes code, I’d like to see it builds on everything.

@jkotas
Copy link
Member

jkotas commented Sep 1, 2022

Tests are not starting. :-/

Yes, it is a planned eng system downtime. Check your email.

@VSadov
Copy link
Member Author

VSadov commented Sep 1, 2022

Yes, it is a planned eng system downtime. Check your email.

I am one of those who miss the email from the airline about 1 hour earlier departure for the last flight to the mainland. (they did not wait :)

@VSadov VSadov closed this Sep 1, 2022
@VSadov VSadov reopened this Sep 1, 2022
@VSadov
Copy link
Member Author

VSadov commented Sep 1, 2022

Thanks!!

@VSadov VSadov merged commit 5e7cfbf into dotnet:main Sep 1, 2022
@VSadov VSadov deleted the stubRegs branch September 1, 2022 20:04
@VSadov
Copy link
Member Author

VSadov commented Sep 1, 2022

/backport to release/7.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2022

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2974601463

@ghost ghost locked as resolved and limited conversation to collaborators Oct 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants