Page MenuHomePhabricator

[AArch64][SVE][NEON] Add NEON-SVE-Bridge intrinsics
ClosedPublic

Authored by MattDevereau on Nov 29 2021, 8:15 AM.

Details

Summary

Adds svset_neonq, svget_neonq, svdup_neonq AArch64 intrinsics.

These are described in the ACLE specification:
https://github.com/ARM-software/acle/pull/72

Diff Detail

Event Timeline

MattDevereau created this revision.Nov 29 2021, 8:15 AM
MattDevereau requested review of this revision.Nov 29 2021, 8:15 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 29 2021, 8:15 AM
paulwalker-arm added inline comments.Nov 29 2021, 8:32 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1325 ↗(On Diff #390371)

Can you extract this into its own patch as it's really not relevant to the rest of the patch and is currently missing tests. Presumably llvm/test/CodeGen/AArch64/sve-insert-vector.ll needs updating?

clang/test/CodeGen/aarch64_neon_sve_bridge_intrinsics/acle_neon_sve_bridge_dup_neonq.c
9

Do you also want to test the overloaded forms here via SVE_OVERLOADED_FORMS?

clang/include/clang/Basic/BuiltinsAArch64NeonSVEBridge.def
3

Looks like these aren't correct, the number should indicate the number of lanes, and it should be 16 for an s8. q indicates scalable, which is also not right for this builtin.

The code which parses these strings can be found here:

https://github.com/llvm/llvm-project/blob/14c4051122bf4070d624b82189f1093758ecdf69/clang/lib/AST/ASTContext.cpp#L10417

MattDevereau added inline comments.Nov 30 2021, 7:02 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1325 ↗(On Diff #390371)

i've been adding some tests to assert this block of code. i've got tests for insert(vscale x n x bfloat, n x bfloat, idx) and insert(vscale x n x bfloat, vscale x n x bfloat, idx).
the n = 4 and n = 8 tests are fine, but n = 2 for insert(vscale x 2 x bfloat, 2 x bfloat, idx) fails an assertion. i've had a quick poke around but haven't seen an obvious reason why its failing, should I worry about this and spend more time on it or just submit the tests i've already got for 4bf16 and 8bf16?

Matt added a subscriber: Matt.Nov 30 2021, 7:46 AM

updated builtin signatures in clang/include/clang/Basic/BuiltinsAArch64NeonSVEBridge.def
removed irrelevant change in llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
ran clang-format

MattDevereau marked an inline comment as done.Nov 30 2021, 8:17 AM
MattDevereau marked an inline comment as done.
paulwalker-arm added inline comments.Nov 30 2021, 8:36 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1325 ↗(On Diff #390371)

Obviously it would be nice for all combinations to work but that's not something you have to fix if it's not directly affecting what you need.

I've checked and it seems 2 x half doesn't work out of the box either so it sounds reasonable to me for your new bfloat handling to mirror the existing supported half use cases only.

peterwaller-arm added inline comments.Dec 1 2021, 1:46 AM
clang/include/clang/Basic/BuiltinsAArch64NeonSVEBridge_cg.def
37

The second argument is a 'flags' field and these values don't look right.

Refs:

The flags are a generated enum and live in clang/include/clang/Basic/arm_sve_typeflags.inc -- I think you'll need to #include this with LLVM_GET_SVE_ELTTYPES defined, and then you can write it symbolically rather than using a literal numeric value.

MattDevereau marked an inline comment as done.Dec 2 2021, 6:32 AM
MattDevereau added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
1325 ↗(On Diff #390371)

updated SVEMAP2 types
added overloadable intrinsics
updated BUILTIN function signatures

run clang format to fix test macro

replace numbers in BuiltinsAArch64NeonSVEBridge_cg.def with SVETypeFlags enum values

MattDevereau marked an inline comment as done.Dec 6 2021, 3:40 AM

clang-format is upset about this ordering:

#include "clang/Basic/arm_sve_builtin_cg.inc"
#include "clang/Basic/BuiltinsAArch64NeonSVEBridge_cg.def"

but swapping the order causes all SVE tests to fail. I'm ignoring the clang-format error for this

clang/include/clang/Basic/BuiltinsAArch64NeonSVEBridge_cg.def
37

SVETypeFlags enums are already available by this stage so there's no need for any includes. I've replaced the numbers with the enums

MattDevereau marked 2 inline comments as done.Dec 6 2021, 3:41 AM
peterwaller-arm accepted this revision.Dec 8 2021, 7:28 AM

LGTM once D115259 has landed.

This revision is now accepted and ready to land.Dec 8 2021, 7:28 AM