linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*

PATCH Superseded Arnd Bergmann 7 participants 10 messages Feb 25, 2021 – Feb 26, 2021 Clang/LLVM build support

lore

Share & tools

lore

Arnd Bergmann

author

2021-02-25 16:50 UTC #0 lore raw

PATCH 1/1 linux/compiler-clang.h: define HAVE_BUILTIN_BSWAP*

From: Arnd Bergmann <[email protected]>

Separating compiler-clang.h from compiler-gcc.h inadventently dropped the definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling back to the open-coded version and hoping that the compiler detects it.

Since all versions of clang support the __builtin_bswap interfaces, add back the flags and have the headers pick these up automatically.

This results in a 4% improvement of compilation speed for arm defconfig.

Fixes 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")

Signed-off-by Arnd Bergmann <[email protected]>

---
 include/linux/compiler-clang.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

include/linux/compiler-clang.h +10 -0

index 6478bff6fcc2..bbfa9ff6a2ec 100644
@@ -33,6 +33,16 @@
 #define __no_sanitize_thread
 #endif
 
+/*
+ * sparse (__CHECKER__) pretends to be gcc, but can't do constant
+ * folding in __builtin_bswap*() (yet), so don't set these for it.
+ */
+#if defined(CONFIG_ARCH_USE_BUILTIN_BSWAP) && !defined(__CHECKER__)
+#define __HAVE_BUILTIN_BSWAP32__
+#define __HAVE_BUILTIN_BSWAP64__
+#define __HAVE_BUILTIN_BSWAP16__
+#endif /* CONFIG_ARCH_USE_BUILTIN_BSWAP && !__CHECKER__ */
+
 #if __has_feature(undefined_behavior_sanitizer)
 /* GCC does not have __SANITIZE_UNDEFINED__ */
 #define __no_sanitize_undefined \

Signature · 1 line

2.29.2

Nathan Chancellor

maintainer Reviewed-by on v1 1/1

2021-02-25 19:41 UTC #1 lore raw

Quoted 6 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
Separating compiler-clang.h from compiler-gcc.h inadventently dropped the definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling back to the open-coded version and hoping that the compiler detects it.

Quoted 7 lines

Since all versions of clang support the __builtin_bswap interfaces, add back the flags and have the headers pick these up automatically.
This results in a 4% improvement of compilation speed for arm defconfig.
Fixes 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
Signed-off-by Arnd Bergmann <[email protected]>

Reviewed-by Nathan Chancellor <[email protected]>

Quoted 28 lines

---
 include/linux/compiler-clang.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

include/linux/compiler-clang.h +10 -0

index 6478bff6fcc2..bbfa9ff6a2ec 100644
@@ -33,6 +33,16 @@
 #define __no_sanitize_thread
 #endif
 
+/*
+ * sparse (__CHECKER__) pretends to be gcc, but can't do constant
+ * folding in __builtin_bswap*() (yet), so don't set these for it.
+ */
+#if defined(CONFIG_ARCH_USE_BUILTIN_BSWAP) && !defined(__CHECKER__)
+#define __HAVE_BUILTIN_BSWAP32__
+#define __HAVE_BUILTIN_BSWAP64__
+#define __HAVE_BUILTIN_BSWAP16__
+#endif /* CONFIG_ARCH_USE_BUILTIN_BSWAP && !__CHECKER__ */
+
 #if __has_feature(undefined_behavior_sanitizer)
 /* GCC does not have __SANITIZE_UNDEFINED__ */
 #define __no_sanitize_undefined \

Signature · 1 line

2.29.2

Kees Cook

Reviewed-by on v1 1/1

2021-02-25 20:05 UTC #2 lore raw

In reply to Arnd Bergmann

Quoted 6 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
Separating compiler-clang.h from compiler-gcc.h inadventently dropped the definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling back to the open-coded version and hoping that the compiler detects it.

Quoted 7 lines

Since all versions of clang support the __builtin_bswap interfaces, add back the flags and have the headers pick these up automatically.
This results in a 4% improvement of compilation speed for arm defconfig.
Fixes 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
Signed-off-by Arnd Bergmann <[email protected]>

Cc [email protected]

Reviewed-by Kees Cook <[email protected]>

Signature · 1 line

Kees Cook

Andrew Morton

2021-02-25 20:07 UTC #3 lore raw

Quoted 10 lines · On Thu, 25 Feb 2021 12:03:48 -0800 Kees Cook <[email protected]> wrote:

Quoted 9 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
Separating compiler-clang.h from compiler-gcc.h inadventently dropped the definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling back to the open-coded version and hoping that the compiler detects it.
Since all versions of clang support the __builtin_bswap interfaces, add back the flags and have the headers pick these up automatically.

Quoted 6 lines

Quoted 4 lines
This results in a 4% improvement of compilation speed for arm defconfig.
Fixes 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
Signed-off-by Arnd Bergmann <[email protected]>
Cc [email protected]

I figured 4% better compile time isn't significant enough to justify a backport. Thoughts?

Quoted 1 line

Reviewed-by Kees Cook <[email protected]>

Kees Cook

2021-02-25 20:15 UTC #4 lore raw

Quoted 14 lines · Andrew Morton

Quoted 12 lines · On Thu, 25 Feb 2021 12:03:48 -0800 Kees Cook <[email protected]> wrote:
Quoted 11 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
Separating compiler-clang.h from compiler-gcc.h inadventently dropped the definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling back to the open-coded version and hoping that the compiler detects it.
Since all versions of clang support the __builtin_bswap interfaces, add back the flags and have the headers pick these up automatically.
This results in a 4% improvement of compilation speed for arm defconfig.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Fixes 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
Signed-off-by Arnd Bergmann <[email protected]>
Cc [email protected]
I figured 4% better compile time isn't significant enough to justify a backport. Thoughts?

It's a trivial change, so I think it's worth it?

Quoted 3 lines

Quoted 2 lines
Reviewed-by Kees Cook <[email protected]>

Signature · 1 line

Kees Cook

Nathan Chancellor

maintainer

2021-02-25 20:21 UTC #5 lore raw

Quoted 18 lines · Kees Cook

Quoted 17 lines · Andrew Morton
Quoted 15 lines · On Thu, 25 Feb 2021 12:03:48 -0800 Kees Cook <[email protected]> wrote:
Quoted 13 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
Separating compiler-clang.h from compiler-gcc.h inadventently dropped the definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling back to the open-coded version and hoping that the compiler detects it.
Since all versions of clang support the __builtin_bswap interfaces, add back the flags and have the headers pick these up automatically.
This results in a 4% improvement of compilation speed for arm defconfig.
Fixes 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
Signed-off-by Arnd Bergmann <[email protected]>

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Cc [email protected]
I figured 4% better compile time isn't significant enough to justify a backport. Thoughts?
It's a trivial change, so I think it's worth it?

Indeed. Any wins that we can get with compile time, we should take. Clang is being widely used in production systems now so I feel like with a trivial change plus user visible impact, it should be backported.

Not to mention that the generated code in theory should be better because it is the compiler's builtin, rather than a hand rolled one, AND this is technically a regression, given that it worked before compiler.h was split.

Cheers,
Nathan

Quoted 6 lines

Quoted 3 lines
Quoted 2 lines
Reviewed-by Kees Cook <[email protected]>
Signature · 1 line
Kees Cook

Miguel Ojeda

2021-02-26 03:59 UTC #6 lore raw

Quoted 9 lines · Nathan Chancellor

Indeed. Any wins that we can get with compile time, we should take. Clang is being widely used in production systems now so I feel like with a trivial change plus user visible impact, it should be backported.
Not to mention that the generated code in theory should be better because it is the compiler's builtin, rather than a hand rolled one, AND this is technically a regression, given that it worked before compiler.h was split.

Compilation speed shouldn't be an argument for a stable change unless it is something egregious like a 50% that may affect users or tightly timed CIs.

Fixing an important runtime regression is a stronger argument, but the patch doesn't show what the effects are, so it isn't justified (yet).

Please note that this kind of change touches a lot of code all over the place, which could always trigger other runtime regressions or even bad codegen (yes, very unlikely, but it happens).

Cheers,
Miguel

Miguel Ojeda

Acked-by

2021-02-26 03:35 UTC #7 lore raw

In reply to Kees Cook

Quoted 2 lines · Kees Cook

It's a trivial change, so I think it's worth it?

It is simple, but it is not trivial since it may change codegen.

Acked-by Miguel Ojeda <[email protected]>

Cheers,
Miguel

Nick Desaulniers

reviewer Acked-by

2021-02-25 21:11 UTC #8 lore raw

In reply to Andrew Morton

Quoted 15 lines · Andrew Morton

Quoted 12 lines · On Thu, 25 Feb 2021 12:03:48 -0800 Kees Cook <[email protected]> wrote:
Quoted 11 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
Separating compiler-clang.h from compiler-gcc.h inadventently dropped the definitions of the three HAVE_BUILTIN_BSWAP macros, which requires falling back to the open-coded version and hoping that the compiler detects it.
Since all versions of clang support the __builtin_bswap interfaces, add back the flags and have the headers pick these up automatically.
This results in a 4% improvement of compilation speed for arm defconfig.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Fixes 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
Signed-off-by Arnd Bergmann <[email protected]>
Cc [email protected]
I figured 4% better compile time isn't significant enough to justify a backport. Thoughts?

If I made a mistake in 815f0ddb346c, then it would be important to correct it since 815f0ddb346c has existed for a few stable branches (first landed in v4.19-rc1).

Acked-by Nick Desaulniers <[email protected]>

Quoted 3 lines

Quoted 2 lines
Reviewed-by Kees Cook <[email protected]>

Signature · 2 lines

Thanks,
~Nick Desaulniers

Luc Van Oostenryck

2021-02-26 07:48 UTC #9 lore raw

In reply to Arnd Bergmann

Quoted 13 lines · Arnd Bergmann

From: Arnd Bergmann <[email protected]>
Fixes 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive")
Signed-off-by Arnd Bergmann <[email protected]>
---
 include/linux/compiler-clang.h | 10 ++++++++++
 1 file changed, 10 insertions(+)
include/linux/compiler-clang.h +0 -0
index 6478bff6fcc2..bbfa9ff6a2ec 100644

include/linux/compiler-clang.h line 39

Quoted 7 lines

include/linux/compiler-clang.h +4 -0

@@ -33,6 +33,16 @@
 #define __no_sanitize_thread
 #endif
 
+/*
+ * sparse (__CHECKER__) pretends to be gcc, but can't do constant
+ * folding in __builtin_bswap*() (yet), so don't set these for it.
+ */

This is not true anymore since 2017. Also, a much recent version of Sparse is needed for _Generic(), for example).

Can you remove the comment and the test for __CHECKER__?

Best regards,
-- Luc