lkml.org 
[lkml]   [2022]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RESEND PATCH v4 1/2] x86/asm/bitops: ffs: use __builtin_ffs to evaluate constant expressions
    Hi Borislav,

    On Thu. 11 Aug 2022 at 23:59, Borislav Petkov <bp@alien8.de> wrote:
    > On Sun, Jul 24, 2022 at 12:15:20AM +0900, Vincent Mailhol wrote:
    > > For x86_64, the current ffs() implementation does not produce
    > > optimized code when called with a constant expression. On the
    > > contrary, the __builtin_ffs() function of both GCC and clang is able
    > > to simplify the expression into a single instruction.
    > >
    > > * Example *
    > >
    > > Let's consider two dummy functions foo() and bar() as below:
    > >
    > > | #include <linux/bitops.h>
    > > | #define CONST 0x01000000
    >
    > Those code examples you can simply indent with two spaces.
    >
    > > In both examples, we clearly see the benefit of using __builtin_ffs()
    >
    > Who's "we"?
    >
    > Please use passive voice in your commit message: no "we" or "I", etc,
    > and describe your changes in imperative mood.
    >
    > > instead of the kernel's asm implementation for constant expressions.
    > >
    > > However, for non constant expressions, the ffs() asm version of the
    > > kernel remains better for x86_64 because, contrary to GCC, it doesn't
    > > emit the CMOV assembly instruction, c.f. [1] (noticeably, clang is
    > > able optimize out the CMOV call).
    > >
    > > This patch uses the __builtin_constant_p() to select between the
    >
    > Avoid having "This patch" or "This commit" in the commit message. It is
    > tautologically useless.
    >
    > Also, do
    >
    > $ git grep 'This patch' Documentation/process
    >
    > for more details.
    >
    > > kernel's ffs() and the __builtin_ffs() depending on whether the
    > > argument is constant or not.
    >
    > In general, you don't have to say what the patch does - that should be
    > visible from the diff. The more important part is the *why*. And that
    > you do.
    >
    > Rest looks ok.

    Thank you for the review!
    I addressed all your comments and sent a v5:
    https://lore.kernel.org/all/20220812114438.1574-1-mailhol.vincent@wanadoo.fr/


    Yours sincerely,
    Vincent Mailhol

    \
     
     \ /
      Last update: 2022-08-12 13:56    [W:6.618 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site