[ARM64EC] Fix compilation of intrin.h in ARM64EC mode. #87717

efriedma-quic · 2024-04-04T21:57:40Z

intrin.h checks for x86_64. But the "x86_64" define is also defined for ARM64EC, and we don't support all the intrinsics in ARM64EC mode. Fix the preprocessor checks to handle this correctly. (If we actually need some of these intrinsics in ARM64EC mode, we can revisit later.)

Not exactly sure how I didn't run into this issue before now... I think I've built code that requires these headers, but maybe not since the define fix landed.

llvmbot · 2024-04-04T21:58:10Z

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-clang

Author: Eli Friedman (efriedma-quic)

Changes

intrin.h checks for x86_64. But the "x86_64" define is also defined for ARM64EC, and we don't support all the intrinsics in ARM64EC mode. Fix the preprocessor checks to handle this correctly. (If we actually need some of these intrinsics in ARM64EC mode, we can revisit later.)

Not exactly sure how I didn't run into this issue before now... I think I've built code that requires these headers, but maybe not since the define fix landed.

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

3 Files Affected:

(modified) clang/lib/Headers/intrin.h (+9-8)
(modified) clang/lib/Headers/intrin0.h (+2-2)
(modified) clang/test/Headers/ms-intrin.cpp (+12-2)

diff --git a/clang/lib/Headers/intrin.h b/clang/lib/Headers/intrin.h
index fd27955fbe002d..40073b4a9f9c31 100644
--- a/clang/lib/Headers/intrin.h
+++ b/clang/lib/Headers/intrin.h
@@ -18,7 +18,7 @@
 #include <intrin0.h>
 
 /* First include the standard intrinsics. */
-#if defined(__i386__) || defined(__x86_64__)
+#if defined(__i386__) || (defined(__x86_64__) && !defined(__arm64ec__))
 #include <x86intrin.h>
 #endif
 
@@ -166,7 +166,7 @@ unsigned __int32 xbegin(void);
 void _xend(void);
 
 /* These additional intrinsics are turned on in x64/amd64/x86_64 mode. */
-#ifdef __x86_64__
+#if defined(__x86_64__) && !defined(__arm64ec__)
 void __addgsbyte(unsigned long, unsigned char);
 void __addgsdword(unsigned long, unsigned long);
 void __addgsqword(unsigned long, unsigned __int64);
@@ -236,7 +236,8 @@ __int64 _mul128(__int64, __int64, __int64 *);
 /*----------------------------------------------------------------------------*\
 |* movs, stos
 \*----------------------------------------------------------------------------*/
-#if defined(__i386__) || defined(__x86_64__)
+
+#if defined(__i386__) || (defined(__x86_64__) && !defined(__arm64ec__))
 static __inline__ void __DEFAULT_FN_ATTRS __movsb(unsigned char *__dst,
                                                   unsigned char const *__src,
                                                   size_t __n) {
@@ -305,7 +306,7 @@ static __inline__ void __DEFAULT_FN_ATTRS __stosw(unsigned short *__dst,
                        : "memory");
 }
 #endif
-#ifdef __x86_64__
+#if defined(__x86_64__) && !defined(__arm64ec__)
 static __inline__ void __DEFAULT_FN_ATTRS __movsq(
     unsigned long long *__dst, unsigned long long const *__src, size_t __n) {
   __asm__ __volatile__("rep movsq"
@@ -324,7 +325,7 @@ static __inline__ void __DEFAULT_FN_ATTRS __stosq(unsigned __int64 *__dst,
 /*----------------------------------------------------------------------------*\
 |* Misc
 \*----------------------------------------------------------------------------*/
-#if defined(__i386__) || defined(__x86_64__)
+#if defined(__i386__) || (defined(__x86_64__) && !defined(__arm64ec__))
 static __inline__ void __DEFAULT_FN_ATTRS __halt(void) {
   __asm__ volatile("hlt");
 }
@@ -339,7 +340,7 @@ static __inline__ void __DEFAULT_FN_ATTRS __nop(void) {
 /*----------------------------------------------------------------------------*\
 |* MS AArch64 specific
 \*----------------------------------------------------------------------------*/
-#if defined(__aarch64__)
+#if defined(__aarch64__) || defined(__arm64ec__)
 unsigned __int64 __getReg(int);
 long _InterlockedAdd(long volatile *Addend, long Value);
 __int64 _InterlockedAdd64(__int64 volatile *Addend, __int64 Value);
@@ -383,7 +384,7 @@ void __cdecl __prefetch(void *);
 /*----------------------------------------------------------------------------*\
 |* Privileged intrinsics
 \*----------------------------------------------------------------------------*/
-#if defined(__i386__) || defined(__x86_64__)
+#if defined(__i386__) || (defined(__x86_64__)  && !defined(__arm64ec__))
 static __inline__ unsigned __int64 __DEFAULT_FN_ATTRS
 __readmsr(unsigned long __register) {
   // Loads the contents of a 64-bit model specific register (MSR) specified in
@@ -397,7 +398,6 @@ __readmsr(unsigned long __register) {
   __asm__ ("rdmsr" : "=d"(__edx), "=a"(__eax) : "c"(__register));
   return (((unsigned __int64)__edx) << 32) | (unsigned __int64)__eax;
 }
-#endif
 
 static __inline__ unsigned __LPTRINT_TYPE__ __DEFAULT_FN_ATTRS __readcr3(void) {
   unsigned __LPTRINT_TYPE__ __cr3_val;
@@ -413,6 +413,7 @@ static __inline__ void __DEFAULT_FN_ATTRS
 __writecr3(unsigned __INTPTR_TYPE__ __cr3_val) {
   __asm__ ("mov {%0, %%cr3|cr3, %0}" : : "r"(__cr3_val) : "memory");
 }
+#endif
 
 #ifdef __cplusplus
 }
diff --git a/clang/lib/Headers/intrin0.h b/clang/lib/Headers/intrin0.h
index 31f362ec84d5c5..338535d3d51736 100644
--- a/clang/lib/Headers/intrin0.h
+++ b/clang/lib/Headers/intrin0.h
@@ -15,7 +15,7 @@
 #ifndef __INTRIN0_H
 #define __INTRIN0_H
 
-#ifdef __x86_64__
+#if defined(__x86_64__) && !defined(__arm64ec__)
 #include <adcintrin.h>
 #endif
 
@@ -55,7 +55,7 @@ unsigned __int64 __shiftright128(unsigned __int64 _LowPart,
                                  unsigned char _Shift);
 #endif
 
-#if defined(__x86_64__) || defined(__i386__)
+#if defined(__i386__) || (defined(__x86_64__) && !defined(__arm64ec__))
 void _mm_pause(void);
 #endif
 
diff --git a/clang/test/Headers/ms-intrin.cpp b/clang/test/Headers/ms-intrin.cpp
index d3b6a1a278dab8..cb7cd47956205c 100644
--- a/clang/test/Headers/ms-intrin.cpp
+++ b/clang/test/Headers/ms-intrin.cpp
@@ -18,6 +18,16 @@
 // RUN:     -ffreestanding -fsyntax-only -Werror \
 // RUN:     -isystem %S/Inputs/include %s
 
+// RUN: %clang_cc1 -triple aarch64--windows \
+// RUN:     -fms-compatibility -fms-compatibility-version=17.00 \
+// RUN:     -ffreestanding -fsyntax-only -Werror \
+// RUN:     -isystem %S/Inputs/include %s
+
+// RUN: %clang_cc1 -triple arm64ec--windows \
+// RUN:     -fms-compatibility -fms-compatibility-version=17.00 \
+// RUN:     -ffreestanding -fsyntax-only -Werror \
+// RUN:     -isystem %S/Inputs/include %s
+
 // REQUIRES: x86-registered-target
 
 // intrin.h needs size_t, but -ffreestanding prevents us from getting it from
@@ -41,7 +51,7 @@ void f() {
   __stosd(0, 0, 0);
   __stosw(0, 0, 0);
 
-#ifdef _M_X64
+#if defined(_M_X64) && !defined(_M_ARM64EC)
   __movsq(0, 0, 0);
   __stosq(0, 0, 0);
 #endif
@@ -49,7 +59,7 @@ void f() {
   int info[4];
   __cpuid(info, 0);
   __cpuidex(info, 0, 0);
-#if defined(_M_X64) || defined(_M_IX86)
+#if (defined(_M_X64) && !defined(_M_ARM64EC)) || defined(_M_IX86)
   _xgetbv(0);
 #endif
   __halt();

efriedma-quic · 2024-04-04T21:59:20Z

CC @bylaws @mstorsjo @cjacek @MaxEW707 @CaseyCarter

github-actions · 2024-04-04T22:00:29Z

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

intrin.h checks for x86_64. But the "x86_64" define is also defined for ARM64EC, and we don't support all the intrinsics in ARM64EC mode. Fix the preprocessor checks to handle this correctly. (If we actually need some of these intrinsics in ARM64EC mode, we can revisit later.) Not exactly sure how I didn't run into this issue before now... I think I've built code that requires these headers, but maybe not since the define fix landed.

MaxEW707 · 2024-04-05T00:36:29Z

clang/lib/Headers/intrin.h

@@ -413,6 +413,7 @@ static __inline__ void __DEFAULT_FN_ATTRS
 __writecr3(unsigned __INTPTR_TYPE__ __cr3_val) {
  __asm__ ("mov {%0, %%cr3|cr3, %0}" : : "r"(__cr3_val) : "memory");
 }
+#endif


Nice catch. We should probably also ifdef these function declarations near the top of this file.

https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/intrin.h#L96
https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/intrin.h#L126

That chunk of declarations near the top of the file needs a lot of work to match the actual implementation; I'd prefer to just leave it for now.

MaxEW707 · 2024-04-05T01:18:55Z

I didn't realize ARM64EC was supported by clang.

You are missing one #if defined(__x86_64__) && !defined(__arm64ec__) check here, https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/intrin0.h#L47.
Doesn't appear to be anyway to comment on lines that aren't in the diff view on github :(.

If we actually need some of these intrinsics in ARM64EC mode, we can revisit later.

For intrin0.h looks like we are missing some intrinsics that msvc stl does use.

Adding defined(__arm64ec__) to the following lines should match intrin0.h that is shipped with msvc 1939.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/intrin0.h#L30
https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/intrin0.h#L86

Since we are intending to revisit adding missing intrinsics later I will do that over this upcoming weekend when I have some free time if you decide not to do it in this PR :).

cjacek

(If we actually need some of these intrinsics in ARM64EC mode, we can revisit later.)

FWIW, most of them are provided by MSVC/Windows SDK via softintrin.h (included from intrin.h) as functions, not actual intrisics.

FreddyLeaf · 2024-10-12T01:19:20Z

clang/lib/Headers/intrin0.h

@@ -44,7 +44,7 @@ unsigned char _InterlockedCompareExchange128_rel(__int64 volatile *_Destination,
                                                 __int64 *_ComparandResult);
 #endif

-#ifdef __x86_64__
+#ifdef __x86_64__ && !defined(__arm64ec__)


Hello, we found compile errors on this line:

(47,19): error: extra tokens at end of #ifdef directive [-Werror,-Wextra-tokens]

Seems like this line forgot to change #ifdef when changed into expression

@FreddyLeaf Curiosity how are you running clang. Since intrin0.h is treated as a system header warnings should be suppressed.

I couldn't repro the warning in my local testing due to intrin0.h being found via a system include path.

Sorry I don't have the reproducer, either. The scene supposed to be building V-Ray with MSbuild on Windows.

MaxEW707 · 2024-10-12T02:08:10Z

I put up a PR with a fix, #112066.

This should probably go into 19.1.2 but I am not familiar with that process.
If someone with more experience in the project can give guidance there that would be appreciated :).

Moon6688 · 2024-10-12T02:35:15Z

intrin.h checks for x86_64. But the "x86_64" define is also defined for ARM64EC, and we don't support all the intrinsics in ARM64EC mode. Fix the preprocessor checks to handle this correctly. (If we actually need some of these intrinsics in ARM64EC mode, we can revisit later.)

Not exactly sure how I didn't run into this issue before now... I think I've built code that requires these headers, but maybe not since the define fix landed.

…rective (#112066) Fixes #87717.

…rective (llvm#112066) Fixes llvm#87717. (cherry picked from commit 9bf68c2)

…rective (llvm#112066) Fixes llvm#87717.

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

efriedma-quic force-pushed the arm64ec-intrin branch from 4de12f2 to f18163b Compare April 4, 2024 23:22

MaxEW707 reviewed Apr 5, 2024

View reviewed changes

[fixup] add a few more __arm64ec__ checks.

0436dc0

MaxEW707 approved these changes Apr 5, 2024

View reviewed changes

cjacek approved these changes Apr 8, 2024

View reviewed changes

efriedma-quic merged commit 1950ebd into llvm:main Apr 8, 2024
4 checks passed

FreddyLeaf reviewed Oct 12, 2024

View reviewed changes

MaxEW707 mentioned this pull request Oct 12, 2024

[Headers] [ARM64EC] Fix extra tokens inside intrin0.h preprocessor directive #112066

Merged

MaxEW707 added a commit that referenced this pull request Oct 14, 2024

[Headers] [ARM64EC] Fix extra tokens inside intrin0.h preprocessor di…

9bf68c2

…rective (#112066) Fixes #87717.

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Oct 14, 2024

[Headers] [ARM64EC] Fix extra tokens inside intrin0.h preprocessor di…

006c85c

…rective (llvm#112066) Fixes llvm#87717. (cherry picked from commit 9bf68c2)

tru pushed a commit to llvmbot/llvm-project that referenced this pull request Oct 15, 2024

[Headers] [ARM64EC] Fix extra tokens inside intrin0.h preprocessor di…

6892827

…rective (llvm#112066) Fixes llvm#87717. (cherry picked from commit 9bf68c2)

DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024

[Headers] [ARM64EC] Fix extra tokens inside intrin0.h preprocessor di…

77fcd48

…rective (llvm#112066) Fixes llvm#87717.

bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 17, 2024

[Headers] [ARM64EC] Fix extra tokens inside intrin0.h preprocessor di…

9f9e7f6

…rective (llvm#112066) Fixes llvm#87717.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ARM64EC] Fix compilation of intrin.h in ARM64EC mode. #87717

[ARM64EC] Fix compilation of intrin.h in ARM64EC mode. #87717

efriedma-quic commented Apr 4, 2024

llvmbot commented Apr 4, 2024 •

edited

Loading

efriedma-quic commented Apr 4, 2024

github-actions bot commented Apr 4, 2024 •

edited

Loading

MaxEW707 Apr 5, 2024

efriedma-quic Apr 5, 2024

MaxEW707 commented Apr 5, 2024 •

edited

Loading

cjacek left a comment

FreddyLeaf Oct 12, 2024

MaxEW707 Oct 14, 2024

FreddyLeaf Oct 14, 2024

MaxEW707 commented Oct 12, 2024 •

edited

Loading

Moon6688 commented Oct 12, 2024

[ARM64EC] Fix compilation of intrin.h in ARM64EC mode. #87717

[ARM64EC] Fix compilation of intrin.h in ARM64EC mode. #87717

Conversation

efriedma-quic commented Apr 4, 2024

llvmbot commented Apr 4, 2024 • edited Loading

efriedma-quic commented Apr 4, 2024

github-actions bot commented Apr 4, 2024 • edited Loading

MaxEW707 Apr 5, 2024

Choose a reason for hiding this comment

efriedma-quic Apr 5, 2024

Choose a reason for hiding this comment

MaxEW707 commented Apr 5, 2024 • edited Loading

cjacek left a comment

Choose a reason for hiding this comment

FreddyLeaf Oct 12, 2024

Choose a reason for hiding this comment

MaxEW707 Oct 14, 2024

Choose a reason for hiding this comment

FreddyLeaf Oct 14, 2024

Choose a reason for hiding this comment

MaxEW707 commented Oct 12, 2024 • edited Loading

Moon6688 commented Oct 12, 2024

llvmbot commented Apr 4, 2024 •

edited

Loading

github-actions bot commented Apr 4, 2024 •

edited

Loading

MaxEW707 commented Apr 5, 2024 •

edited

Loading

MaxEW707 commented Oct 12, 2024 •

edited

Loading