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

Fix win-arm64 native varargs ABI #90712

Merged

Conversation

BruceForstall
Copy link
Member

@BruceForstall BruceForstall commented Aug 17, 2023

SIMD vector types should be passed in integer registers for win-arm64 native varargs. This might require splitting a Vector128 between x7 and stack.

See https://learn.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=msvc-170#addendum-variadic-functions

Fixes #89595

@ghost ghost assigned BruceForstall Aug 17, 2023
@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 Aug 17, 2023
@ghost
Copy link

ghost commented Aug 17, 2023

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

Issue Details

SIMD vector types should be passed in integer registers. This might require splitting a Vector128 between x7 and stack.

Author: BruceForstall
Assignees: BruceForstall
Labels:

area-CodeGen-coreclr

Milestone: -

@BruceForstall
Copy link
Member Author

Only failure is known failure #90593

@BruceForstall
Copy link
Member Author

No spmi asm diffs -- negligible TP change on arm64.

@BruceForstall
Copy link
Member Author

@jakobbotsch PTAL
cc @dotnet/jit-contrib

@jkotas
Copy link
Member

jkotas commented Aug 17, 2023

Does VM-side implementation of calling conventions (https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/callingconvention.h) work as expected for this case?

if (varTypeIsSIMD(type))
{
// Vectors also get passed in int registers. Use TYP_INT.
return TYP_INT;
Copy link
Member

Choose a reason for hiding this comment

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

Is the size mismatch here fine?

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out the size doesn't matter; the only thing that matters is INT (use general-purpose registers) or non-INT (use floaing-point/SIMD registers).

@BruceForstall
Copy link
Member Author

Does VM-side implementation of calling conventions (https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/callingconvention.h) work as expected for this case?

It looks like it might already be covered:

#ifdef _WIN32
if (this->IsVarArg() && m_idxGenReg < 8)
{
// Address the Windows ARM64 varargs case where an arg is split between regs and stack.
// This can happen in the varargs case because the first 64 bytes of the stack are loaded
// into x0-x7, and any remaining stack arguments are placed normally.
int argOfs = TransitionBlock::GetOffsetOfArgumentRegisters() + m_idxGenReg * 8;
// Increase m_ofsStack to account for the space used for the remainder of the arg after
// registers are filled.
m_ofsStack += cbArg + (m_idxGenReg - 8) * TARGET_POINTER_SIZE;
// We used up the remaining reg slots.
m_idxGenReg = 8;
return argOfs;
}
else
#endif

One thing that wasn't handled on the JIT is passing/receiving any Vector to/in win-arm64 varargs functions. How could I test this case through the callingconvention.h code?

@jkotas
Copy link
Member

jkotas commented Aug 17, 2023

callingconvention.h is only used for GC stackroot reporting.

It may be useful to add a variant of the test with interleaving scalars and object refences in the vararg signature, so that we would catch the situation where the GC stackroot reporting does not report the varargs arguments correctly.

SIMD vector types should be passed in integer registers.
This might require splitting a Vector128 between x7 and stack.
@BruceForstall
Copy link
Member Author

callingconvention.h is only used for GC stackroot reporting.

It may be useful to add a variant of the test with interleaving scalars and object refences in the vararg signature, so that we would catch the situation where the GC stackroot reporting does not report the varargs arguments correctly.

I stepped through callingconvention.h and saw the correct behavior occuring with the current code.

I added a few addtional test cases to the test.

Add additional varargs arguments after the vector

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@BruceForstall BruceForstall merged commit 31234a8 into dotnet:main Aug 18, 2023
137 checks passed
@BruceForstall BruceForstall deleted the FixWinArm64VarargsVectorSplit branch August 18, 2023 23:08
@ghost ghost locked as resolved and limited conversation to collaborators Sep 18, 2023
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.

JIT: win-arm64 native varargs doesn't correctly split incoming vectors between register and stack
3 participants