[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKwvOdnDi9GmLL8iRDe1-LEnpbFWFPAsPqB5PWpnpGK8+0rYAw@mail.gmail.com>
Date: Thu, 10 Nov 2022 11:20:21 -0800
From: Nick Desaulniers <ndesaulniers@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Vincent Mailhol <mailhol.vincent@...adoo.fr>, 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 Mon, Nov 7, 2022 at 1:39 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Sun, Nov 06, 2022 at 06:51:05PM +0900, Vincent Mailhol wrote:
> > 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.
>
> I'm thinking this is the same old clang-sucks-at-"rm" constraints and
> *really* should not be a reason to change things. Clang should get fixed
> already.
Well messing up constant folding for all compilers absolutely should
be a reason!
I did get a chance to speak with some colleagues more about this at
the LLVM developer meeting during the past 2 days. We have some ideas
on approaches that might work. There's some higher priority features
we're working on first, but I suspect we'll be able to visit that
issue soon. It's a pretty tricky dance between instruction selection
and register allocation.
>
> > 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")
>
> I would much prefer consistently with 146034fed6ee.
The bottom of this file arch/x86/include/asm/bitops.h is full of
#include <asm-generic/bitops/*.h>
--
Thanks,
~Nick Desaulniers
Powered by blists - more mailing lists