lkml.org 
[lkml]   [2022]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 08/22] bitops: introduce MANY_BITS() macro
On Tue, May 10, 2022 at 05:54:13PM +0000, David Laight wrote:
> From: Alexei Starovoitov
> > Sent: 10 May 2022 17:51
> ...
> > +/* Return: nonzero if 2 or more bits are set */
> > +#define MANY_BITS(n) ((n) & ((n) - 1))
>
> You can't have a macro that expands its argument twice.

Yes, I'll fix it.

> ...
> > > static inline __attribute__((const))
> > > bool is_power_of_2(unsigned long n)
> > > {
> > > - return (n != 0 && ((n & (n - 1)) == 0));
> > > + return n != 0 && !MANY_BITS(n);
> > > }
> >
> > Please don't. Open coded version is much easier to read.

To me the human-readable version is easier to read. Still, if you thing
that n & (n - 1) is simpler, what for this function is needed at all?

> Especially if you remove all the spare parenthesis.
> return n && !(n & (n - 1));
>
> I bet a lot of callers know the value is non-zero.
>
> I suspect you'll find at least one caller that uses
> is_power_of_2() assuming it is !(n & (n - 1)) and
> so is wrong for zero.

Another thing is that despite __attribute__(const), gcc sometimes doesn't
recognize it as constant expression, and people have to workaround it.
XTENSA is the example for 1st case, and for the 2nd:

arch/powerpc/mm/init-common.c:
unsigned long minalign = max(MAX_PGTABLE_INDEX_SIZE + 1,
HUGEPD_SHIFT_MASK + 1);

/* It would be nice if this was a BUILD_BUG_ON(), but at the
* moment, gcc doesn't seem to recognize is_power_of_2
* as a constant expression, so so much for that. */
BUG_ON(!is_power_of_2(minalign));

This convinced me that we need a simple macro that is decoupled with
pow_2 semantics and can be used in another macros like BUILD_BUG_ON().

Thanks,
Yury

\
 
 \ /
  Last update: 2022-05-10 21:12    [W:0.102 / U:0.408 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site