[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKwvOdnzx0Y-vmFsRS6=rCveqAJVn6hXEutdn_ow=jbtNeQcCA@mail.gmail.com>
Date: Thu, 10 Nov 2022 11:06:10 -0800
From: Nick Desaulniers <ndesaulniers@...gle.com>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc: x86@...nel.org, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>,
Thomas Gleixner <tglx@...utronix.de>,
linux-kernel@...r.kernel.org, Yury Norov <yury.norov@...il.com>,
llvm@...ts.linux.dev, Borislav Petkov <bp@...e.de>
Subject: Re: [PATCH v1 1/2] x86/asm/bitops: Replace __fls() by its generic
builtin implementation
On Sun, Nov 6, 2022 at 1:51 AM Vincent Mailhol
<mailhol.vincent@...adoo.fr> wrote:
>
> Below snippet:
>
> #include <linux/bitops.h>
>
> unsigned int foo(unsigned long word)
> {
> return __fls(word);
> }
>
> produces this on GCC 12.1.0:
>
> 0000000000000000 <foo>:
> 0: f3 0f 1e fa endbr64
> 4: e8 00 00 00 00 call 9 <foo+0x9>
> 9: 53 push %rbx
> a: 48 89 fb mov %rdi,%rbx
> d: e8 00 00 00 00 call 12 <foo+0x12>
> 12: 48 0f bd c3 bsr %rbx,%rax
> 16: 5b pop %rbx
> 17: 31 ff xor %edi,%edi
> 19: e9 00 00 00 00 jmp 1e <foo+0x1e>
>
> and that on clang 14.0.6:
>
> 0000000000000000 <foo>:
> 0: f3 0f 1e fa endbr64
> 4: e8 00 00 00 00 call 9 <foo+0x9>
> 9: 53 push %rbx
> a: 50 push %rax
> b: 48 89 fb mov %rdi,%rbx
> e: e8 00 00 00 00 call 13 <foo+0x13>
> 13: 48 89 1c 24 mov %rbx,(%rsp)
> 17: 48 0f bd 04 24 bsr (%rsp),%rax
> 1c: 48 83 c4 08 add $0x8,%rsp
> 20: 5b pop %rbx
> 21: c3 ret
>
> The implementation from <asm-generic/bitops/builtin-__fls.h> [1]
> produces the exact same code on GCC and below code on clang:
>
> 0000000000000000 <foo>:
> 0: f3 0f 1e fa endbr64
> 4: e8 00 00 00 00 call 9 <foo+0x9>
> 9: 53 push %rbx
> a: 48 89 fb mov %rdi,%rbx
> d: e8 00 00 00 00 call 12 <foo+0x12>
> 12: 48 0f bd c3 bsr %rbx,%rax
> 16: 5b pop %rbx
> 17: c3 ret
>
> The builtin implementation is better for two reasons:
>
> 1/ it saves two instructions on clang (a push and a stack pointer
> decrement) because of a useless tentative to save rax.
>
> 2/ when used on constant expressions, the compiler is only able to
> fold the builtin version (c.f. [2]).
>
> For those two reasons, replace the assembly implementation by its
> builtin counterpart.
>
> [1] https://elixir.bootlin.com/linux/v6.0/source/include/asm-generic/bitops/builtin-__fls.h
>
> [2] commit 146034fed6ee ("x86/asm/bitops: Use __builtin_ffs() to evaluate constant expressions")
>
> CC: Borislav Petkov <bp@...e.de>
> CC: Nick Desaulniers <ndesaulniers@...gle.com>
> CC: Yury Norov <yury.norov@...il.com>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
LGTM; thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com>
> ---
> arch/x86/include/asm/bitops.h | 14 +-------------
> 1 file changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
> index 2edf68475fec..a31453d7686d 100644
> --- a/arch/x86/include/asm/bitops.h
> +++ b/arch/x86/include/asm/bitops.h
> @@ -285,19 +285,7 @@ static __always_inline unsigned long variable_ffz(unsigned long word)
> (unsigned long)__builtin_ctzl(~word) : \
> variable_ffz(word))
>
> -/*
> - * __fls: find last set bit in word
> - * @word: The word to search
> - *
> - * Undefined if no set bit exists, so code should check against 0 first.
> - */
> -static __always_inline unsigned long __fls(unsigned long word)
> -{
> - asm("bsr %1,%0"
> - : "=r" (word)
> - : "rm" (word));
> - return word;
> -}
> +#include <asm-generic/bitops/builtin-__fls.h>
>
> #undef ADDR
>
> --
> 2.37.4
>
--
Thanks,
~Nick Desaulniers
Powered by blists - more mailing lists