Skip to contentSkip to conversation

Get rid of all NEON intrinsics stdint.h type hacks

PATCH In review Ard Biesheuvel 3 participants 7 messages Oct 29 – Nov 1

Subsystems (7)
lore
Share & tools
lore
#0 lore raw

cover Get rid of all NEON intrinsics stdint.h type hacks

From: Ard Biesheuvel <[email protected]>

Recent Clang has become more finicky when it comes to overriding the type macros that the freestanding stdint.h consumes when defining the uintXX_t types, and so it is time to finally get rid of this entirely.

So instead of making stdint.h (which comes from the compiler when -ffreestanding is set on the command line) agree with the kernel's linux/types.h header, avoid the latter entirely in the compilation units that actually use the NEON intrinsics (which are always leaf functions that are not tightly coupled to kernel APIs).

Note that the disagreements are about long vs. long long on arm64, and long vs. int on 32-bit ARM, so never about types that actually have conflicting representations.

Cc Christoph Hellwig <[email protected]>
Cc Eric Biggers <[email protected]>
Cc Nathan Chancellor <[email protected]>

Ard Biesheuvel (4):
lib/crc: Don't include <linux/types.h> in NEON intrinsics code
xor: arm/arm64: Move NEON xor_gen() dispatch into the glue code
arm64: Drop stdint.h type overrides from asm/neon-intrinsics.h
ARM: Drop stdint.h type overrides from asm/neon-intrinsics.h

 Documentation/arch/arm/kernel_mode_neon.rst | 10 +++--
 arch/arm/include/asm/neon-intrinsics.h      | 43 +++-----------------
 arch/arm64/include/asm/neon-intrinsics.h    | 24 +++--------
 lib/crc/crc64-neon.c                        | 10 ++---
 lib/raid/xor/arm/xor-neon-glue.c            |  3 ++
 lib/raid/xor/arm/xor-neon.h                 | 16 +++++++-
 lib/raid/xor/arm64/xor-eor3.c               | 15 ++-----
 lib/raid/xor/arm64/xor-neon-glue.c          |  5 +++
 lib/raid/xor/arm64/xor-neon.h               | 31 ++++++++++++--
 lib/raid/xor/xor-neon.c                     | 16 ++------
 10 files changed, 78 insertions(+), 95 deletions(-)
Signature · 1 line
2.56.0.313.g2479322e68-goog
#1 lore raw

PATCH 1/4 lib/crc: Don't include <linux/types.h> in NEON intrinsics code

From: Ard Biesheuvel <[email protected]>

The kernel's definitions of uint64_t and int64_t in <linux/types.h> are based on [unsigned] long long, whereas the ones provided by <stdint.h>, which is included by <arm_neon.h>, are based on [unsigned] long on arm64. On 32-bit ARM, a similar mismatch exists for intptr_t.

Including both into the same compilation unit currently only works because asm/neon-intrinsics.h overrides the predefined macros that the compiler's freestanding <stdint.h> uses to define these types. In order to be able to get rid of those overrides, stop including <linux/types.h> into the CRC64 NEON intrinsics code.

Instead, use <stddef.h> for the definition of size_t, and rely on <asm/neon-intrinsics.h> to provide the u8 and u64 types used in the prototype of crc64_nvme_neon(), which is shared with code that does include <linux/types.h>. Use uint64_t for the arrays of constants that are passed to vld1q_u64(), so that they match its prototype regardless of how uint64_t is defined.

No change in generated code.

---
 lib/crc/crc64-neon.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
lib/crc/crc64-neon.c +5 -5
index 4753fb94a4be..1267df1310be 100644
@@ -3,7 +3,7 @@
  * Accelerated CRC64 (NVMe) using ARM NEON C intrinsics
  */
 
-#include <linux/types.h>
+#include <stddef.h>
 #include <asm/neon-intrinsics.h>
 
 #include "crc64-neon.h"
@@ -11,11 +11,11 @@
 u64 crc64_nvme_neon(u64 crc, const u8 *p, size_t len);
 
 /* x^191 mod G, x^127 mod G */
-static const u64 fold_consts_val[2] = { 0xeadc41fd2ba3d420ULL,
-					0x21e9761e252621acULL };
+static const uint64_t fold_consts_val[2] = { 0xeadc41fd2ba3d420ULL,
+					     0x21e9761e252621acULL };
 /* floor(x^127 / G), (G - x^64) / x */
-static const u64 bconsts_val[2] = { 0x27ecfa329aef9f77ULL,
-				    0x34d926535897936aULL };
+static const uint64_t bconsts_val[2] = { 0x27ecfa329aef9f77ULL,
+					 0x34d926535897936aULL };
 
 u64 crc64_nvme_neon(u64 crc, const u8 *p, size_t len)
 {
Signature · 1 line
2.56.0.313.g2479322e68-goog
#2 lore raw

PATCH 2/4 xor: arm/arm64: Move NEON xor_gen() dispatch into the glue code

In reply to Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>

The NEON intrinsics code in xor-neon.c and arm64/xor-eor3.c includes xor_impl.h for the definition of __DO_XOR_BLOCKS(), which pulls in <linux/types.h> via <linux/init.h> and <linux/minmax.h>. This only works because asm/neon-intrinsics.h overrides the compiler's predefined macros that <stdint.h> uses to define the C99 types, so that they match the kernel's definitions in <linux/types.h>.

So instead, follow the approach taken by LoongArch, and instantiate the dispatch routines using DO_XOR_BLOCKS() in the glue code, which already includes the kernel headers, and does not use NEON intrinsics itself. This requires the NEON routines that handle a fixed number of sources to be exposed to the glue code, which means that the arm64 glue code can use __xor_neon_2() directly for the EOR3 flavor as well, and so the __xor_eor3_2() alias can be dropped.

This makes the NEON intrinsics code independent of the kernel's own headers, which will allow the macro overrides in asm/neon-intrinsics.h to be removed.

---
 lib/raid/xor/arm/xor-neon-glue.c   |  3 ++
 lib/raid/xor/arm/xor-neon.h        | 16 ++++++++--
 lib/raid/xor/arm64/xor-eor3.c      | 15 ++--------
 lib/raid/xor/arm64/xor-neon-glue.c |  5 ++++
 lib/raid/xor/arm64/xor-neon.h      | 31 +++++++++++++++++---
 lib/raid/xor/xor-neon.c            | 16 +++-------
 6 files changed, 56 insertions(+), 30 deletions(-)
lib/raid/xor/arm/xor-neon-glue.c +3 -0
index cea39e019904..783474bfe81e 100644
@@ -5,6 +5,9 @@
 #include "xor_impl.h"
 #include "xor_arch.h"
 
+DO_XOR_BLOCKS(neon_inner, __xor_neon_2, __xor_neon_3, __xor_neon_4,
+		__xor_neon_5);
+
 static void xor_gen_neon(void *dest, void **srcs, unsigned int src_cnt,
 		unsigned int bytes)
 {
lib/raid/xor/arm/xor-neon.h +14 -2
index 406e0356f05b..5592dec4de50 100644
@@ -3,5 +3,17 @@
 extern struct xor_block_template xor_block_arm4regs;
 extern struct xor_block_template xor_block_neon;
 
-void xor_gen_neon_inner(void *dest, void **srcs, unsigned int src_cnt,
-		unsigned int bytes);
+void __xor_neon_2(unsigned long bytes, unsigned long * __restrict p1,
+		const unsigned long * __restrict p2);
+void __xor_neon_3(unsigned long bytes, unsigned long * __restrict p1,
+		const unsigned long * __restrict p2,
+		const unsigned long * __restrict p3);
+void __xor_neon_4(unsigned long bytes, unsigned long * __restrict p1,
+		const unsigned long * __restrict p2,
+		const unsigned long * __restrict p3,
+		const unsigned long * __restrict p4);
+void __xor_neon_5(unsigned long bytes, unsigned long * __restrict p1,
+		const unsigned long * __restrict p2,
+		const unsigned long * __restrict p3,
+		const unsigned long * __restrict p4,
+		const unsigned long * __restrict p5);
lib/raid/xor/arm64/xor-eor3.c +3 -12
index e44016c363f1..41bb577b63eb 100644
@@ -1,14 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
-#include <linux/cache.h>
 #include <asm/neon-intrinsics.h>
-#include "xor_impl.h"
-#include "xor_arch.h"
 #include "xor-neon.h"
 
-extern void __xor_eor3_2(unsigned long bytes, unsigned long * __restrict p1,
-		const unsigned long * __restrict p2);
-
 static inline uint64x2_t eor3(uint64x2_t p, uint64x2_t q, uint64x2_t r)
 {
 	uint64x2_t res;
@@ -19,7 +13,7 @@ static inline uint64x2_t eor3(uint64x2_t p, uint64x2_t q, uint64x2_t r)
 	return res;
 }
 
-static void __xor_eor3_3(unsigned long bytes, unsigned long * __restrict p1,
+void __xor_eor3_3(unsigned long bytes, unsigned long * __restrict p1,
 		const unsigned long * __restrict p2,
 		const unsigned long * __restrict p3)
 {
@@ -53,7 +47,7 @@ static void __xor_eor3_3(unsigned long bytes, unsigned long * __restrict p1,
 	} while (--lines > 0);
 }
 
-static void __xor_eor3_4(unsigned long bytes, unsigned long * __restrict p1,
+void __xor_eor3_4(unsigned long bytes, unsigned long * __restrict p1,
 		const unsigned long * __restrict p2,
 		const unsigned long * __restrict p3,
 		const unsigned long * __restrict p4)
@@ -96,7 +90,7 @@ static void __xor_eor3_4(unsigned long bytes, unsigned long * __restrict p1,
 	} while (--lines > 0);
 }
 
-static void __xor_eor3_5(unsigned long bytes, unsigned long * __restrict p1,
+void __xor_eor3_5(unsigned long bytes, unsigned long * __restrict p1,
 		const unsigned long * __restrict p2,
 		const unsigned long * __restrict p3,
 		const unsigned long * __restrict p4,
@@ -141,6 +135,3 @@ static void __xor_eor3_5(unsigned long bytes, unsigned long * __restrict p1,
 		dp5 += 8;
 	} while (--lines > 0);
 }
-
-__DO_XOR_BLOCKS(eor3_inner, __xor_eor3_2, __xor_eor3_3, __xor_eor3_4,
-		__xor_eor3_5);
lib/raid/xor/arm64/xor-neon-glue.c +5 -0
index f0284f86feb4..ae133d699649 100644
@@ -9,6 +9,11 @@
 #include "xor_arch.h"
 #include "xor-neon.h"
 
+DO_XOR_BLOCKS(neon_inner, __xor_neon_2, __xor_neon_3, __xor_neon_4,
+		__xor_neon_5);
+DO_XOR_BLOCKS(eor3_inner, __xor_neon_2, __xor_eor3_3, __xor_eor3_4,
+		__xor_eor3_5);
+
 #define XOR_TEMPLATE(_name)						\
 static void xor_gen_##_name(void *dest, void **srcs, unsigned int src_cnt, \
 		unsigned int bytes)					\
lib/raid/xor/arm64/xor-neon.h +27 -4
index 514699ba8f5f..7d7fb96343df 100644
@@ -1,6 +1,29 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
-void xor_gen_neon_inner(void *dest, void **srcs, unsigned int src_cnt,
-		unsigned int bytes);
-void xor_gen_eor3_inner(void *dest, void **srcs, unsigned int src_cnt,
-		unsigned int bytes);
+void __xor_neon_2(unsigned long bytes, unsigned long * __restrict p1,
+		const unsigned long * __restrict p2);
+void __xor_neon_3(unsigned long bytes, unsigned long * __restrict p1,
+		const unsigned long * __restrict p2,
+		const unsigned long * __restrict p3);
+void __xor_neon_4(unsigned long bytes, unsigned long * __restrict p1,
+		const unsigned long * __restrict p2,
+		const unsigned long * __restrict p3,
+		const unsigned long * __restrict p4);
+void __xor_neon_5(unsigned long bytes, unsigned long * __restrict p1,
+		const unsigned long * __restrict p2,
+		const unsigned long * __restrict p3,
+		const unsigned long * __restrict p4,
+		const unsigned long * __restrict p5);
+
+void __xor_eor3_3(unsigned long bytes, unsigned long * __restrict p1,
+		const unsigned long * __restrict p2,
+		const unsigned long * __restrict p3);
+void __xor_eor3_4(unsigned long bytes, unsigned long * __restrict p1,
+		const unsigned long * __restrict p2,
+		const unsigned long * __restrict p3,
+		const unsigned long * __restrict p4);
+void __xor_eor3_5(unsigned long bytes, unsigned long * __restrict p1,
+		const unsigned long * __restrict p2,
+		const unsigned long * __restrict p3,
+		const unsigned long * __restrict p4,
+		const unsigned long * __restrict p5);
lib/raid/xor/xor-neon.c +4 -12
index c7c3cf634e23..6b2fd14f5041 100644
@@ -4,12 +4,11 @@
  * Copyright (C) 2018,Tianjin KYLIN Information Technology Co., Ltd.
  */
 
-#include "xor_impl.h"
 #include "xor-neon.h"
 
 #include <asm/neon-intrinsics.h>
 
-static void __xor_neon_2(unsigned long bytes, unsigned long * __restrict p1,
+void __xor_neon_2(unsigned long bytes, unsigned long * __restrict p1,
 		const unsigned long * __restrict p2)
 {
 	uint64_t *dp1 = (uint64_t *)p1;
@@ -36,7 +35,7 @@ static void __xor_neon_2(unsigned long bytes, unsigned long * __restrict p1,
 	} while (--lines > 0);
 }
 
-static void __xor_neon_3(unsigned long bytes, unsigned long * __restrict p1,
+void __xor_neon_3(unsigned long bytes, unsigned long * __restrict p1,
 		const unsigned long * __restrict p2,
 		const unsigned long * __restrict p3)
 {
@@ -72,7 +71,7 @@ static void __xor_neon_3(unsigned long bytes, unsigned long * __restrict p1,
 	} while (--lines > 0);
 }
 
-static void __xor_neon_4(unsigned long bytes, unsigned long * __restrict p1,
+void __xor_neon_4(unsigned long bytes, unsigned long * __restrict p1,
 		const unsigned long * __restrict p2,
 		const unsigned long * __restrict p3,
 		const unsigned long * __restrict p4)
@@ -117,7 +116,7 @@ static void __xor_neon_4(unsigned long bytes, unsigned long * __restrict p1,
 	} while (--lines > 0);
 }
 
-static void __xor_neon_5(unsigned long bytes, unsigned long * __restrict p1,
+void __xor_neon_5(unsigned long bytes, unsigned long * __restrict p1,
 		const unsigned long * __restrict p2,
 		const unsigned long * __restrict p3,
 		const unsigned long * __restrict p4,
@@ -170,10 +169,3 @@ static void __xor_neon_5(unsigned long bytes, unsigned long * __restrict p1,
 		dp5 += 8;
 	} while (--lines > 0);
 }
-
-__DO_XOR_BLOCKS(neon_inner, __xor_neon_2, __xor_neon_3, __xor_neon_4,
-		__xor_neon_5);
-
-#ifdef CONFIG_ARM64
-extern typeof(__xor_neon_2) __xor_eor3_2 __alias(__xor_neon_2);
-#endif
Signature · 1 line
2.56.0.313.g2479322e68-goog
#3 lore raw

PATCH 3/4 arm64: Drop stdint.h type overrides from asm/neon-intrinsics.h

In reply to Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>

None of the users of NEON intrinsics include <linux/types.h> anymore, and so there is no longer a need to override __INT64_TYPE__ and __UINT64_TYPE__ in order to make the definitions of int64_t and uint64_t provided by the compiler's freestanding <stdint.h> compatible with the kernel's. So drop the overrides.

This also means that uint64_t now matches the type that Clang uses for 64-bit quantities in the prototypes of its NEON builtins, and so the -Wincompatible-pointer-types warnings that are currently being disabled for all NEON intrinsics code will no longer occur. So drop the pragma that disables them as well.

---
 arch/arm64/include/asm/neon-intrinsics.h | 24 ++++----------------
 1 file changed, 5 insertions(+), 19 deletions(-)
arch/arm64/include/asm/neon-intrinsics.h +5 -19
index 5f13505d39fc..43cdf4d16170 100644
@@ -6,23 +6,13 @@
 #ifndef __ASM_NEON_INTRINSICS_H
 #define __ASM_NEON_INTRINSICS_H
 
-#include <asm-generic/int-ll64.h>
-
 /*
- * In the kernel, u64/s64 are [un]signed long long, not [un]signed long.
- * So by redefining these macros to the former, we can force gcc-stdint.h
- * to define uint64_t / in64_t in a compatible manner.
+ * <arm_neon.h> includes <stdint.h>, which defines some C99 types differently
+ * from <linux/types.h>, so the latter must not be included into code that uses
+ * NEON intrinsics. The kernel's u8/u16/u32/u64 types can be used alongside the
+ * C99 ones, so provide those instead.
  */
-
-#ifdef __INT64_TYPE__
-#undef __INT64_TYPE__
-#define __INT64_TYPE__		long long
-#endif
-
-#ifdef __UINT64_TYPE__
-#undef __UINT64_TYPE__
-#define __UINT64_TYPE__		unsigned long long
-#endif
+#include <asm-generic/int-ll64.h>
 
 /*
  * genksyms chokes on the ARM NEON instrinsics system header, but we
@@ -33,8 +23,4 @@
 #include <arm_neon.h>
 #endif
 
-#ifdef CONFIG_CC_IS_CLANG
-#pragma clang diagnostic ignored "-Wincompatible-pointer-types"
-#endif
-
 #endif /* __ASM_NEON_INTRINSICS_H */
Signature · 1 line
2.56.0.313.g2479322e68-goog
#4 lore raw

PATCH 4/4 ARM: Drop stdint.h type overrides from asm/neon-intrinsics.h

In reply to Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>

None of the users of NEON intrinsics include <linux/types.h> anymore, and so there is no longer a need to override the compiler's predefined macros that are used to define the C99 [u]int32_t and [u]intptr_t types in order to make them compatible with the kernel's definitions. So drop the overrides from asm/neon-intrinsics.h.

Update the documentation accordingly.

---
 Documentation/arch/arm/kernel_mode_neon.rst | 10 +++--
 arch/arm/include/asm/neon-intrinsics.h      | 43 +++-----------------
 2 files changed, 12 insertions(+), 41 deletions(-)
Documentation/arch/arm/kernel_mode_neon.rst +7 -3
index 1efb6d35b7bd..73331f17b2c0 100644
@@ -121,6 +121,10 @@ observe the following in addition to the rules above:
 * Compile the unit containing the NEON intrinsics with '-ffreestanding' so GCC
   uses its builtin version of <stdint.h> (this is a C99 header which the kernel
   does not supply);
-* Do not include <arm_neon.h> directly: instead, include <asm/neon-intrinsics.h>,
-  which tweaks some macro definitions so that system headers can be included
-  safely.
+* Do not include <arm_neon.h> directly: instead, include <asm/neon-intrinsics.h>;
+* Do not include <linux/types.h>, or any other kernel header that includes it,
+  into the same compilation unit: some of the C99 types it defines (e.g.,
+  intptr_t) are incompatible with the ones defined by <stdint.h>. The kernel's
+  own u8, u16, u32 and u64 types are provided by <asm/neon-intrinsics.h>, and
+  can be used safely in the prototypes of functions that are called from code
+  that does include <linux/types.h>.
arch/arm/include/asm/neon-intrinsics.h +5 -38

51 lines not on this page · raw message

Signature · 1 line
2.56.0.313.g2479322e68-goog
maintainer Tested-by on the v1 series
#5 lore raw
In reply to Ard Biesheuvel

Hi Ard,

Quoted 3 lines · Ard Biesheuvel

Recent Clang has become more finicky when it comes to overriding the type macros that the freestanding stdint.h consumes when defining the uintXX_t types, and so it is time to finally get rid of this entirely.

Just as information for other folks:

https://github.com/llvm/llvm-project/pull/227722#issuecomment-6065215071

Quoted 5 lines

So instead of making stdint.h (which comes from the compiler when -ffreestanding is set on the command line) agree with the kernel's linux/types.h header, avoid the latter entirely in the compilation units that actually use the NEON intrinsics (which are always leaf functions that are not tightly coupled to kernel APIs).

I had not even considered this as an option, thanks a lot for exploring it and sending this series as a result. I definitely think this is the preferred route forward based on my discussions with clang folks upstream since they were quite unhappy with stdint.h being overridden in this manner.

Quoted 9 lines

Note that the disagreements are about long vs. long long on arm64, and long vs. int on 32-bit ARM, so never about types that actually have conflicting representations.

Ard Biesheuvel (4):
lib/crc: Don't include <linux/types.h> in NEON intrinsics code
xor: arm/arm64: Move NEON xor_gen() dispatch into the glue code
arm64: Drop stdint.h type overrides from asm/neon-intrinsics.h
ARM: Drop stdint.h type overrides from asm/neon-intrinsics.h

This passes my arm and arm64 test matrix with LLVM 23.1.4 and a version of LLVM 24 with the breaking change mentioned above. This will be needed in stable but I don't mind taking a look at the backports when the time comes.

Signature · 2 lines
Cheers,
Nathan
maintainer Reviewed-by on the v1 series
#6 lore raw
In reply to Ard Biesheuvel
Quoted 20 lines · Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>

Recent Clang has become more finicky when it comes to overriding the type macros that the freestanding stdint.h consumes when defining the uintXX_t types, and so it is time to finally get rid of this entirely.

So instead of making stdint.h (which comes from the compiler when -ffreestanding is set on the command line) agree with the kernel's linux/types.h header, avoid the latter entirely in the compilation units that actually use the NEON intrinsics (which are always leaf functions that are not tightly coupled to kernel APIs).

Note that the disagreements are about long vs. long long on arm64, and long vs. int on 32-bit ARM, so never about types that actually have conflicting representations.

Cc Christoph Hellwig <[email protected]>
Cc Eric Biggers <[email protected]>
Cc Nathan Chancellor <[email protected]>
Quoted 5 lines

Ard Biesheuvel (4):
lib/crc: Don't include <linux/types.h> in NEON intrinsics code
xor: arm/arm64: Move NEON xor_gen() dispatch into the glue code
arm64: Drop stdint.h type overrides from asm/neon-intrinsics.h
ARM: Drop stdint.h type overrides from asm/neon-intrinsics.h

- Eric