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

[LLVM][SVE] Honour calling convention when using SVE for fixed length vectors. #70847

Merged
merged 2 commits into from
Nov 30, 2023

Conversation

paulwalker-arm
Copy link
Collaborator

@paulwalker-arm paulwalker-arm commented Oct 31, 2023

NOTE: I'm not sure how many of the corner cases are part of the documented ABI but that shouldn't matter because my goal is for -msve-vector-bits to have no affect on the way arguments and returns are processed.

NOTE: I'm forming an idea as to whether this means we can potentially enable fixed length predicate types because my main blocking concern was not wanting to change the existing calling convention surrounding those types.

Copy link

github-actions bot commented Oct 31, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@paulwalker-arm
Copy link
Collaborator Author

Just a rebase, I still need to add tests.

@vfdff
Copy link
Contributor

vfdff commented Nov 4, 2023

hi @paulwalker-arm , If we still prefer the NEON regitster, can we just ignore to addRegisterClass for these fix registers, whose vector length is bigger than 128-bit?

if (Subtarget->useSVEForFixedLengthVectors()) {

@paulwalker-arm
Copy link
Collaborator Author

hi @paulwalker-arm , If we still prefer the NEON regitster, can we just ignore to addRegisterClass for these fix registers, whose vector length is bigger than 128-bit?

if (Subtarget->useSVEForFixedLengthVectors()) {

No because we still want such types to be legal so that in function code generation can use them. This patch just ensures that at function call boundaries the "legal" types are restricted to those which are defined by an ABI.

I've also started thinking about whether this scheme means I can also make fixed length i1 vectors legal so that we can make better use of the predicate registers because there's a few cases where force extending them to i8 vectors causes pretty bad code. This is especially true when crossing basic block boundaries.

@vfdff
Copy link
Contributor

vfdff commented Nov 8, 2023

hi @paulwalker-arm , If we still prefer the NEON regitster, can we just ignore to addRegisterClass for these fix registers, whose vector length is bigger than 128-bit?

if (Subtarget->useSVEForFixedLengthVectors()) {

No because we still want such types to be legal so that in function code generation can use them. This patch just ensures that at function call boundaries the "legal" types are restricted to those which are defined by an ABI.

I've also started thinking about whether this scheme means I can also make fixed length i1 vectors legal so that we can make better use of the predicate registers because there's a few cases where force extending them to i8 vectors causes pretty bad code. This is especially true when crossing basic block boundaries.

thanks very much for your explaination :)

@paulwalker-arm paulwalker-arm changed the title [WIP] - [LLVM][SVE] Honour NEON calling convention when targeting SVE VLS. [LLVM][SVE] Honour calling convention when using SVE for fixed length vectors. Nov 23, 2023
@paulwalker-arm
Copy link
Collaborator Author

  • Rebased.
  • Finalised the support by matching the behaviour for types that should result in scalarisation.
  • Added tests.

@paulwalker-arm
Copy link
Collaborator Author

To aid review I've split the tests into a separate patch, albeit they don't show the existing broken output because many of the tests trigger internal compiler errors and the existing output does not really matter because it's wrong.

Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

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

I agree this sounds like the right direction to take. We probably cant get away with not supporting these argument types with fixed length SVE, and it sounds best to not change the ABI based on having the target feature and sve min vector width.

This seems to mark scalar variables as being passed in gpr registers, where as they should be the first element in a wider vector. See AArch64TargetLowering::getPreferredVectorAction for where it is overridden.

@paulwalker-arm
Copy link
Collaborator Author

paulwalker-arm commented Nov 27, 2023

This seems to mark scalar variables as being passed in gpr registers, where as they should be the first element in a wider vector. See AArch64TargetLowering::getPreferredVectorAction for where it is overridden.

Thanks for the info. This comes from the base version of getVectorTypeBreakdown. However, this gets bypassed for simple EVTs so it looks like I'll need special handing for scalar vectors as well.

…tion.

NOTE: When SVE is enable some of these tests trigger asserts.
@paulwalker-arm
Copy link
Collaborator Author

paulwalker-arm commented Nov 27, 2023

Fallback to calling base versions of getRegisterTypeForCallingConv and getNumRegistersForCallingConv for unit-length vectors. Extended testing to cover unit-length vectors.

Copy link
Collaborator

@davemgreen davemgreen left a comment

Choose a reason for hiding this comment

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

Thanks. From what I can tell this LGTM.

@paulwalker-arm paulwalker-arm merged commit 4db451a into llvm:main Nov 30, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants