On Wed, Feb 26, 2025 at 10:29 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > On Wed, Feb 26, 2025 at 10:12:47PM -0800, Bill Wendling wrote: > > For both gcc and clang, crc32 builtins generate better code than the > > inline asm. GCC improves, removing unneeded "mov" instructions. Clang > > does the same and unrolls the loops. GCC has no changes on i386, but > > Clang's code generation is vastly improved, due to Clang's "rm" > > constraint issue. > > > > The number of cycles improved by ~0.1% for GCC and ~1% for Clang, which > > is expected because of the "rm" issue. However, Clang's performance is > > better than GCC's by ~1.5%, most likely due to loop unrolling. > > > > Link: https://github.com/llvm/llvm-project/issues/20571#issuecomment-2649330009 > > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx> > > Cc: Ingo Molnar <mingo@xxxxxxxxxx> > > Cc: Borislav Petkov <bp@xxxxxxxxx> > > Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx> > > Cc: x86@xxxxxxxxxx > > Cc: "H. Peter Anvin" <hpa@xxxxxxxxx> > > Cc: Eric Biggers <ebiggers@xxxxxxxxxx> > > Cc: Ard Biesheuvel <ardb@xxxxxxxxxx> > > Cc: Nathan Chancellor <nathan@xxxxxxxxxx> > > Cc: Nick Desaulniers <nick.desaulniers+lkml@xxxxxxxxx> > > Cc: Justin Stitt <justinstitt@xxxxxxxxxx> > > Cc: linux-kernel@xxxxxxxxxxxxxxx > > Cc: linux-crypto@xxxxxxxxxxxxxxx > > Cc: llvm@xxxxxxxxxxxxxxx > > Signed-off-by: Bill Wendling <morbo@xxxxxxxxxx> > > --- > > arch/x86/Makefile | 3 +++ > > arch/x86/lib/crc32-glue.c | 8 ++++---- > > 2 files changed, 7 insertions(+), 4 deletions(-) > > Thanks! A couple concerns, though: > > > diff --git a/arch/x86/Makefile b/arch/x86/Makefile > > index 5b773b34768d..241436da1473 100644 > > --- a/arch/x86/Makefile > > +++ b/arch/x86/Makefile > > @@ -114,6 +114,9 @@ else > > KBUILD_CFLAGS += $(call cc-option,-fcf-protection=none) > > endif > > > > +# Enables the use of CRC32 builtins. > > +KBUILD_CFLAGS += -mcrc32 > > Doesn't this technically allow the compiler to insert CRC32 instructions > anywhere in arch/x86/ without the needed runtime CPU feature check? Normally > when using intrinsics it's necessary to limit the scope of the feature > enablement to match the runtime CPU feature check that is done, e.g. by using > the target function attribute. > I'm not sure if CRC32 instructions will automatically be inserted when not explicitly called, especially since the other vector features are disabled. I wanted to limit enabling this flag for only crc32-glue.c, but my Makefile-fu failed me. The file appears to be compiled twice. But adding __attribute__((target("crc32"))) to the function would be much better. > > diff --git a/arch/x86/lib/crc32-glue.c b/arch/x86/lib/crc32-glue.c > > index 2dd18a886ded..fdb94bff25f4 100644 > > --- a/arch/x86/lib/crc32-glue.c > > +++ b/arch/x86/lib/crc32-glue.c > > @@ -48,9 +48,9 @@ u32 crc32_le_arch(u32 crc, const u8 *p, size_t len) > > EXPORT_SYMBOL(crc32_le_arch); > > > > #ifdef CONFIG_X86_64 > > -#define CRC32_INST "crc32q %1, %q0" > > +#define CRC32_INST __builtin_ia32_crc32di > > #else > > -#define CRC32_INST "crc32l %1, %0" > > +#define CRC32_INST __builtin_ia32_crc32si > > #endif > > Do both gcc and clang consider these builtins to be a stable API, or do they > only guarantee the stability of _mm_crc32_*() from immintrin.h? At least for > the rest of the SSE and AVX stuff, I thought that only the immintrin.h functions > are actually considered stable. > I don't know the answer for this. In general, once we (Clang) create a __builtin_* function it's not going away, because it will break anyone who uses them. (I assume the same is true for GCC.) There's a note in Documentation/arch/x86/x86_64/fsgs.rst in regards to using _{read,write}fsbase_u64() from immintrin.h (see below). I don't know if that's analogous to what I'm doing here, but maybe we should do something similar for crc32intr.h? FSGSBASE instructions compiler support ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GCC version 4.6.4 and newer provide intrinsics for the FSGSBASE instructions. Clang 5 supports them as well. =================== =========================== _readfsbase_u64() Read the FS base register _readfsbase_u64() Read the GS base register _writefsbase_u64() Write the FS base register _writegsbase_u64() Write the GS base register =================== =========================== To utilize these intrinsics <immintrin.h> must be included in the source code and the compiler option -mfsgsbase has to be added. -bw
