arm_neon.h intrinsics should be target-gated, not preprocessor-gated

[davidben]

Clang's intrinsics headers on Arm contain code like:

#if !defined(__ARM_NEON)
#error "NEON support not enabled"
#else

or:

#if __ARM_ARCH >= 8 && defined(__ARM_FEATURE_AES)
#ifdef __LITTLE_ENDIAN__
__ai uint8x16_t vaesdq_u8(uint8x16_t __p0, uint8x16_t __p1) {
...

(Generated by https://github.com/llvm/llvm-project/blob/main/clang/utils/TableGen/NeonEmitter.cpp.)

This means that one can only use Arm intrinsics in TUs that mark the feature as available for the entire intrinsic, e.g. via -march flags. In contrast, the x86 intrinsics are consistently defined, but tagged with __attribute__((__target__("whatever"))):
https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/avx2intrin.h#L18

The x86 variant is both easier to use, as it doesn't require messing with your project's build definition, and safer, since you can just write a target("avx2") function and then gate the unmarked -> marked transition on some suitable CPUID check. (Or perhaps even use multi-versioning, though I believe target attributes are usable even without that.)

In contrast, Clang's Arm story requires messing with build definitions and risks ODR violations. Suppose your NEON-enabled TU included some inline functions, where Clang happened to vectorize code and use NEON instructions. If that copy of the inline function won, the resulting binary would inadvertently require NEON, even if the overall target wasn't meant to require NEON.

GCC fixed their Arm intrinsics, back in 2015, to be target-gated instead. Arm intrinsics would be much more usable if Clang could get parity here.
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=ae5e29239e28818f807cf11775c95c4243d9a256;hp=b8c7c62b2dbbdf355adb56d8250e68222ae0febb

[davidben]

Playing around with godbolt, it looks like making this work for NEON itself may be more involved than just fixing the header:

<source>:5:24: error: 'neon_vector_type' attribute is not supported on targets missing 'neon' or 'mve'; specify an appropriate -march= or -mcpu=
typedef __attribute__((neon_vector_type(4))) uint32_t uint32x4_t;

Not sure about other features, which don't depend on feature-gated types.

[jan-wassenberg]

+1, this is important for SVE and even NEON adoption.
Here's a Compiler Explorer example of this in action using GCC.

[nico]

Seems like a cool feature.

I also ran into this while trying to write a faster SHA256 using intrinsics over at #56121.

Adding a random assortment of folks who touched arm_neon.td and arm_sve.td: @davemgreen @dcandler @sdesmalen-arm , wdyt?

@davidben linked to the GCC change already.

9fc7fb2 is the change that did the corresponding change for the intel intrinsics long ago.

[llvmbot]

@llvm/issue-subscribers-backend-aarch64

[kyrilltkachov]

This looks somewhat related to the function multiversioning work from @ilinpv https://reviews.llvm.org/D127812 but perhaps is not blocked by it?

[davidben]

Yeah, I think they're related but can be done independently. Doing this would make function multiversioning much more useful (otherwise function multiversioning is limited to compiler vectorization, as I understand), but function multiversioning is not necessary for this to be useful, since the application can always manage the dispatch itself.

[davemgreen]

This does sound like a good idea, to help the usability of the intrinsics. GCC uses the same target attributes technique on the functions, then relies on an error happening when inlining those functions when the target features do not match. This is the same method that X86 uses, and seems to work OK for AArch64 from these examples:
https://godbolt.org/z/odWjW43xf

(GCC actually uses a different method for arm_sve.h, where they just define a single pragma that tells the compiler that it should include definitions for all the needed sve acle intrinsics).

Changing the tablegen emitter to use those target attributes as opposed to ifdefs looks fairly straightforward. There are some issues however:

• The Neon functions give errors during inlining, but the SVE definitions use __attribute__((__clang_arm_builtin_alias(__builtin_sve_svmmla_s32))), so do not see the same error until it hits a backend "failed to select" fatal error.
• The types need to be defined. This means defining types like bfloat16_t without bfloat16 support, which currently gives an error.
• GCC spells it target("+aes") (or target("arch=armv9-a")). LLVM spells it target("aes"). We would need to align the two.
• I only looked at changing the SVE emitter. We would need to make sure AArch32 still works correctly, and we may of course run into other problems..

[jan-wassenberg]

@davemgreen thanks for looking into this.
As to the target("+aes") vs target("aes") difference, from my perspective it would be fine to use whichever is more convenient for the compiler. We anyway use different syntax between gcc and clang:
pragma clang attribute push(__attribute__((target(targets_str))), apply_to = function)) vs
GCC target targets_str.

[davemgreen]

OK that's good. Thanks for clarifying. It is something that seems worth fixing in general, as the difference between compilers is likely to trip some people up.