Messages in this thread | | | From | Vincent MAILHOL <> | Date | Thu, 12 May 2022 13:29:47 +0900 | Subject | Re: [PATCH v4 1/2] x86/asm/bitops: ffs: use __builtin_ffs to evaluate constant expressions |
| |
On Thu. 12 May 2022 at 12:02, Joe Perches <joe@perches.com> wrote: > On Thu, 2022-05-12 at 10:18 +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. > [] > > -static __always_inline int ffs(int x) > > +static __always_inline int variable_ffs(int x) > > { > > int r; > > > > @@ -310,6 +299,19 @@ static __always_inline int ffs(int x) > > return r + 1; > > } > > > > +/** > > + * ffs - find first set bit in word > > + * @x: the word to search > > + * > > + * This is defined the same way as the libc and compiler builtin ffs > > + * routines, therefore differs in spirit from the other bitops. > > + * > > + * ffs(value) returns 0 if value is 0 or the position of the first > > + * set bit if value is nonzero. The first (least significant) bit > > + * is at position 1. > > + */ > > +#define ffs(x) (__builtin_constant_p(x) ? __builtin_ffs(x) : variable_ffs(x)) > > How about not defining another function and using parentheses around > the function definition to avoid the macro expansion like: > > #define ffs(x) (__builtin_constant_p(x) ? __builtin_ffs(x) : ffs(x)) > > and > > static __always_inline int (ffs)(int x) > { > etc... > }
Sorry, but I don’t really like this approach.
Main issue I see is that this code will emit a -Wshadow warning.
And using parentheses around the function definition just seems an obscure hack to me. The variable_foo() gives me less headache. Was this pattern ever used anywhere else in the kernel?
Yours sincerely, Vincent Mailhol
| |