[clang][ARM][AArch64] Define intrinsics guarded by __has_builtin on all platforms #128222

sarnex · 2025-02-21T19:51:05Z

Instead of defining ARM ACLE intrinsics only on MSVC and guarding wrapper functions in headers with __has_builtin, universally define the intrinsics as target header builtins.

llvmbot · 2025-02-21T21:34:37Z

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-backend-arm

@llvm/pr-subscribers-backend-x86

Author: Nick Sarnie (sarnex)

Changes

Apply a preemptive workaround for a possible behavior change in __has_builtin as suggested in #121839 and #126324.

Full diff: https://github.com/llvm/llvm-project/pull/128222.diff

1 Files Affected:

(modified) clang/lib/Headers/arm_acle.h (+3)

diff --git a/clang/lib/Headers/arm_acle.h b/clang/lib/Headers/arm_acle.h
index b1dc90f84ad36..fbd0020258722 100644
--- a/clang/lib/Headers/arm_acle.h
+++ b/clang/lib/Headers/arm_acle.h
@@ -27,6 +27,8 @@
 extern "C" {
 #endif
 
+#if !defined(__CUDA__)
+
 /* 7 SYNCHRONIZATION, BARRIER AND HINT INTRINSICS */
 /* 7.3 Memory barriers */
 #if !__has_builtin(__dmb)
@@ -70,6 +72,7 @@ static __inline__ void __attribute__((__always_inline__, __nodebug__)) __yield(v
   __builtin_arm_yield();
 }
 #endif
+#endif // #if !defined(__CUDA__)
 
 #if defined(__ARM_32BIT_STATE) && __ARM_32BIT_STATE
 #define __dbg(t) __builtin_arm_dbg(t)

llvmbot · 2025-02-21T21:34:37Z

@llvm/pr-subscribers-clang

Author: Nick Sarnie (sarnex)

Changes

Apply a preemptive workaround for a possible behavior change in __has_builtin as suggested in #121839 and #126324.

Full diff: https://github.com/llvm/llvm-project/pull/128222.diff

1 Files Affected:

(modified) clang/lib/Headers/arm_acle.h (+3)

diff --git a/clang/lib/Headers/arm_acle.h b/clang/lib/Headers/arm_acle.h
index b1dc90f84ad36..fbd0020258722 100644
--- a/clang/lib/Headers/arm_acle.h
+++ b/clang/lib/Headers/arm_acle.h
@@ -27,6 +27,8 @@
 extern "C" {
 #endif
 
+#if !defined(__CUDA__)
+
 /* 7 SYNCHRONIZATION, BARRIER AND HINT INTRINSICS */
 /* 7.3 Memory barriers */
 #if !__has_builtin(__dmb)
@@ -70,6 +72,7 @@ static __inline__ void __attribute__((__always_inline__, __nodebug__)) __yield(v
   __builtin_arm_yield();
 }
 #endif
+#endif // #if !defined(__CUDA__)
 
 #if defined(__ARM_32BIT_STATE) && __ARM_32BIT_STATE
 #define __dbg(t) __builtin_arm_dbg(t)

clang/lib/Headers/arm_acle.h

jhuber6

Maybe a comment for why we need to check for the CUDA arch, but I'll defer to @Artem-B for the final +1.

Artem-B · 2025-02-21T22:11:17Z

clang/lib/Headers/arm_acle.h

+#if !defined(__CUDA_ARCH__)
+


Unfortunately, this will be observable to any code that happens to need those builtins, and they are actually not provided by the compiler (i.e. __has_builtin(__wfi) would be host on the host).

In that case, any use of __wfi() in the host code will break compilation because host-side code will not be able to find it.

I think a more robust fix would be to provide only declarations for these maybe-builtin functions, so the compiler always sees the correct signature of the functions that are conditionally provided depending on __has_builtin().

Do you mean we unconditionally declare the functions, regardless of compilation mode/host/device/__has_builtin result?

#if __CUDA_ARCH__ // Make sure the declarations here are in sync with the functions provided under #else. <declarations> #else <existing code> #endif

I'm actually wondering if the header needs __has_builtin()... They are coming from the same compiler, and, if they are missing, they are declared in terms of __builtin*() functions.

Looking at their definitions, it appears that those builtins are defined when we're targeting windows:

llvm-project/clang/include/clang/Basic/BuiltinsARM.def

Lines 212 to 220 in 79261d4

LANGBUILTIN(__yield, "v", "", ALL_MS_LANGUAGES)

LANGBUILTIN(__wfe, "v", "", ALL_MS_LANGUAGES)

LANGBUILTIN(__wfi, "v", "", ALL_MS_LANGUAGES)

LANGBUILTIN(__sev, "v", "", ALL_MS_LANGUAGES)

LANGBUILTIN(__sevl, "v", "", ALL_MS_LANGUAGES)

LANGBUILTIN(__dmb, "vUi", "nc", ALL_MS_LANGUAGES)

LANGBUILTIN(__dsb, "vUi", "nc", ALL_MS_LANGUAGES)

LANGBUILTIN(__isb, "vUi", "nc", ALL_MS_LANGUAGES)

If we have to define them on non-windows targets anyways, perhaps a better fix is to let clang define them everywhere, and avoid this special case in the headers.

@compnerd -- do you recall what's the story with these non-builtin builtins here?
They were added specifically for MS targets in 4bddd9d

But five years later, @Bigcheese added these wrappers in the header:
16af23f

@compnerd @Bigcheese Do you mind taking a look at Artem's questions above? Thx

@rnk Reid, would you happen to have an idea what's up with these builtins on Windows?

@rnk Ping on this, thx!

If we have to define them on non-windows targets anyways, perhaps a better fix is to let clang define them everywhere, and avoid this special case in the headers.

Yes, we should go this direction. There are some examples of this for x86 builtins. Here's an example of one I did recently for x86:
ee92122

In this example, we leave behind an _m_prefetchw declaration, and the builtin is defined as a "library builtin" which has an associated header. This forces callers to include the relevant header, in this case arm_acle.h, to make the builtin callable.

thanks, ill try to implement this assuming it's relatively easy

i just pushed a commit which attempts to implement this, please take a look, thanks!

github-actions · 2025-04-17T19:43:36Z

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

Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>

Artem-B · 2025-04-17T21:27:14Z

clang/test/Headers/ms-intrin.cpp

@@ -36,6 +36,28 @@ typedef __SIZE_TYPE__ size_t;

 #include <intrin.h>

+#ifdef __ARM_ACLE
+// arm_acle.h needs some stdint types, but -ffreestanding prevents us from


Shouldn't that be fixed in arm_acle.h itself so it includes the headers with the types it needs?

arm_acle.h already includes the correct header, stdint.h, the problem here is this test uses a custom stdint.h in the Inputs/include directory that doesn't have the required types, a better fix is to just add this code to that custom stdint.h, I'll do that in the next commit, thanks.

Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>

rnk

Thanks, I think this is a good direction.

mstorsjo · 2025-04-22T08:01:12Z

This breaks real-world code with MSVC headers.

Testcase:

#include <windows.h>
#include <winnt.h>
void func(void) { _InstructionSynchronizationBarrier(); }

$ bin/clang-cl --target=aarch64-windows-msvc -c isb.c
isb.c(3,19): error: call to undeclared library function '__isb' with type 'void (unsigned int)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    3 | void func(void) { _InstructionSynchronizationBarrier(); }
      |                   ^
/home/martin/msvc2022-17.13/kits/10/include/10.0.22621.0/um/winnt.h(5717,46): note: expanded from macro '_InstructionSynchronizationBarrier'
 5717 | #define _InstructionSynchronizationBarrier() __isb(_ARM64_BARRIER_SY)
      |                                              ^
isb.c(3,19): note: include the header <arm_acle.h> or explicitly provide a declaration for '__isb'
/home/martin/msvc2022-17.13/kits/10/include/10.0.22621.0/um/winnt.h(5717,46): note: expanded from macro '_InstructionSynchronizationBarrier'
 5717 | #define _InstructionSynchronizationBarrier() __isb(_ARM64_BARRIER_SY)
      |                                              ^
1 error generated.

sarnex · 2025-04-22T14:23:44Z

Investigating, thanks

sarnex · 2025-04-22T21:29:41Z

Fix #136742

sarnex marked this pull request as ready for review February 21, 2025 21:34

llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics labels Feb 21, 2025

sarnex requested review from Artem-B and jhuber6 February 21, 2025 21:34

sarnex mentioned this pull request Feb 21, 2025

[Clang] Add __has_target_builtin macro #126324

Open

jhuber6 reviewed Feb 21, 2025

View reviewed changes

clang/lib/Headers/arm_acle.h Outdated Show resolved Hide resolved

jhuber6 approved these changes Feb 21, 2025

View reviewed changes

Artem-B reviewed Feb 21, 2025

View reviewed changes

sarnex force-pushed the cuda branch from ee84261 to 9a28fa5 Compare April 17, 2025 19:41

llvmbot added backend:ARM backend:AArch64 clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 17, 2025

sarnex changed the title ~~[Clang][ARM] Only try to redefine builtins for non-CUDA~~ [clang][ARM][AArch64] Define intrinsics guarded by __has_builtin on all platforms Apr 17, 2025

sarnex requested review from jhuber6, Artem-B and rnk April 17, 2025 19:42

[clang][ARM] Define intrinsics guarded by __has_builtin on all platforms

86cde46

Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>

sarnex force-pushed the cuda branch from 9a28fa5 to 86cde46 Compare April 17, 2025 19:50

Artem-B reviewed Apr 17, 2025

View reviewed changes

improve fix for test with custom stdint.h

4e6107f

Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>

sarnex requested a review from Artem-B April 18, 2025 15:43

rnk approved these changes Apr 21, 2025

View reviewed changes

Artem-B approved these changes Apr 21, 2025

View reviewed changes

sarnex merged commit de0153d into llvm:main Apr 21, 2025
11 checks passed

This was referenced Apr 22, 2025

[clang][ARM][AArch64] Don't require arm_acle header for universally defined intrinsics #136742

Draft

Revert "[clang][ARM][AArch64] Define intrinsics guarded by __has_builtin on all platforms " #137373

Closed

sarnex mentioned this pull request May 2, 2025

[clang][Sema] Don't warn for implicit uses of builtins in system headers #138205

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][ARM][AArch64] Define intrinsics guarded by __has_builtin on all platforms #128222

[clang][ARM][AArch64] Define intrinsics guarded by __has_builtin on all platforms #128222

sarnex commented Feb 21, 2025 •

edited

Loading

llvmbot commented Feb 21, 2025 •

edited

Loading

llvmbot commented Feb 21, 2025

jhuber6 left a comment

Artem-B Feb 21, 2025

sarnex Feb 21, 2025 •

edited

Loading

Artem-B Feb 21, 2025

Artem-B Feb 21, 2025

sarnex Feb 28, 2025

Artem-B Mar 26, 2025

sarnex Apr 11, 2025

rnk Apr 14, 2025

sarnex Apr 16, 2025

sarnex Apr 17, 2025

github-actions bot commented Apr 17, 2025 •

edited

Loading

Artem-B Apr 17, 2025

sarnex Apr 18, 2025

rnk left a comment

mstorsjo commented Apr 22, 2025

sarnex commented Apr 22, 2025

sarnex commented Apr 22, 2025

	LANGBUILTIN(__yield, "v", "", ALL_MS_LANGUAGES)
	LANGBUILTIN(__wfe, "v", "", ALL_MS_LANGUAGES)
	LANGBUILTIN(__wfi, "v", "", ALL_MS_LANGUAGES)
	LANGBUILTIN(__sev, "v", "", ALL_MS_LANGUAGES)
	LANGBUILTIN(__sevl, "v", "", ALL_MS_LANGUAGES)

	LANGBUILTIN(__dmb, "vUi", "nc", ALL_MS_LANGUAGES)
	LANGBUILTIN(__dsb, "vUi", "nc", ALL_MS_LANGUAGES)
	LANGBUILTIN(__isb, "vUi", "nc", ALL_MS_LANGUAGES)

[clang][ARM][AArch64] Define intrinsics guarded by __has_builtin on all platforms #128222

[clang][ARM][AArch64] Define intrinsics guarded by __has_builtin on all platforms #128222

Conversation

sarnex commented Feb 21, 2025 • edited Loading

llvmbot commented Feb 21, 2025 • edited Loading

llvmbot commented Feb 21, 2025

jhuber6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarnex Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnk left a comment

Choose a reason for hiding this comment

mstorsjo commented Apr 22, 2025

sarnex commented Apr 22, 2025

sarnex commented Apr 22, 2025

sarnex commented Feb 21, 2025 •

edited

Loading

llvmbot commented Feb 21, 2025 •

edited

Loading

sarnex Feb 21, 2025 •

edited

Loading

github-actions bot commented Apr 17, 2025 •

edited

Loading