[PATCH] log2: make is_power_of_2() integer constant expression when possible

[PATCH] log2: make is_power_of_2() integer constant expression when possible

[Jani Nikula]

While is_power_of_2() is an inline function and likely gets optimized
for compile time constant arguments, it still doesn't produce an integer
constant expression that could be used in, say, static data
initialization or case labels.

Make is_power_of_2() an integer constant expression when possible,
otherwise using the inline function to avoid multiple evaluation of the
parameter.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

The alternative would be to define both a function and a macro version
of is_power_of_2(), and let the callers decide what to use.
---
 include/linux/log2.h | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/include/linux/log2.h b/include/linux/log2.h
index 2af7f77866d0..035932f52aeb 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -37,6 +37,14 @@ int __ilog2_u64(u64 n)
 }
 #endif
 
+#define __IS_POWER_OF_2(__n) ((__n) != 0 && (((__n) & ((__n) - 1)) == 0))
+
+static inline __attribute__((const))
+bool __is_power_of_2(unsigned long n)
+{
+	return __IS_POWER_OF_2(n);
+}
+
 /**
  * is_power_of_2() - check if a value is a power of two
  * @n: the value to check
@@ -45,11 +53,8 @@ int __ilog2_u64(u64 n)
  * *not* considered a power of two.
  * Return: true if @n is a power of 2, otherwise false.
  */
-static inline __attribute__((const))
-bool is_power_of_2(unsigned long n)
-{
-	return (n != 0 && ((n & (n - 1)) == 0));
-}
+#define is_power_of_2(n) \
+	__builtin_choose_expr(__builtin_constant_p(n), __IS_POWER_OF_2(n), __is_power_of_2(n))
 
 /**
  * __roundup_pow_of_two() - round up to nearest power of two
-- 
2.20.1

Re: [PATCH] log2: make is_power_of_2() integer constant expression when possible

[Chris Wilson]

Quoting Jani Nikula (2019-03-01 12:52:07)
> While is_power_of_2() is an inline function and likely gets optimized
> for compile time constant arguments, it still doesn't produce an integer
> constant expression that could be used in, say, static data
> initialization or case labels.
> 
> Make is_power_of_2() an integer constant expression when possible,
> otherwise using the inline function to avoid multiple evaluation of the
> parameter.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

It does what it says on the tin and allows for

static const is_pot = is_power_of_two(32);

or in the actual case of interest,

BUILD_BUG_ON(is_power_of_two(x) ? test1(x) : test2(x)).

The only question remains can build_bug.h include ilog2.h and use this
macro instead of having its own copy? Or that may be overkill for a
single commonplace macro.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

Re: [PATCH] log2: make is_power_of_2() integer constant expression when possible

[Andrew Morton]

On Fri,  1 Mar 2019 14:52:07 +0200 Jani Nikula <jani.nikula@intel.com> wrote:

> While is_power_of_2() is an inline function and likely gets optimized
> for compile time constant arguments, it still doesn't produce an integer
> constant expression that could be used in, say, static data
> initialization or case labels.

hm, what code wants to do these things?

> Make is_power_of_2() an integer constant expression when possible,
> otherwise using the inline function to avoid multiple evaluation of the
> parameter.

Spose so.  But I fear that some gcc versions will get it right and
others will mess it up.  While this patch is under test I think it
would be best to also have at least one or two callsites which actually
use the compile-time evaluation feature.  Possible?

Re: [PATCH] log2: make is_power_of_2() integer constant expression when possible

[Jani Nikula]

On Fri, 01 Mar 2019, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri,  1 Mar 2019 14:52:07 +0200 Jani Nikula <jani.nikula@intel.com> wrote:
>
>> While is_power_of_2() is an inline function and likely gets optimized
>> for compile time constant arguments, it still doesn't produce an integer
>> constant expression that could be used in, say, static data
>> initialization or case labels.
>
> hm, what code wants to do these things?
>
>> Make is_power_of_2() an integer constant expression when possible,
>> otherwise using the inline function to avoid multiple evaluation of the
>> parameter.
>
> Spose so.  But I fear that some gcc versions will get it right and
> others will mess it up.  While this patch is under test I think it
> would be best to also have at least one or two callsites which actually
> use the compile-time evaluation feature.  Possible?

I have some drm/i915 patches that have a local compile-time version of
IS_POWER_OF_2(). It'll take a while before they hit Linus' master. This
can wait.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH] log2: make is_power_of_2() integer constant expression when possible

[Jani Nikula]

On Mon, 04 Mar 2019, kbuild test robot <lkp@intel.com> wrote:
> Hi Jani,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on linus/master]
> [also build test WARNING on v5.0 next-20190301]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Jani-Nikula/log2-make-is_power_of_2-integer-constant-expression-when-possible/20190304-153529
> config: microblaze-mmu_defconfig (attached as .config)
> compiler: microblaze-linux-gcc (GCC) 8.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=8.2.0 make.cross ARCH=microblaze 
>
> All warnings (new ones prefixed by >>):
>
>>> arch/microblaze/mm/pgtable.c:185: warning: "is_power_of_2" redefined
>     #define is_power_of_2(x) ((x) != 0 && (((x) & ((x) - 1)) == 0))

Oh wow, a copy of is_power_of_2() introduced 10 years ago, apparently
unused since its introduction.


BR,
Jani.


>     
>    In file included from include/linux/kernel.h:12,
>                     from arch/microblaze/mm/pgtable.c:30:
>    include/linux/log2.h:56: note: this is the location of the previous definition
>     #define is_power_of_2(n) \
>     
>
> vim +/is_power_of_2 +185 arch/microblaze/mm/pgtable.c
>
> 15902bf6 Michal Simek 2009-05-26  183  
> 15902bf6 Michal Simek 2009-05-26  184  /* is x a power of 2? */
> 15902bf6 Michal Simek 2009-05-26 @185  #define is_power_of_2(x)	((x) != 0 && (((x) & ((x) - 1)) == 0))
> 15902bf6 Michal Simek 2009-05-26  186  
>
> :::::: The code at line 185 was first introduced by commit
> :::::: 15902bf63c8332946e5a1f48a72e3ae22874b11b microblaze_mmu_v2: Page table - ioremap - pgtable.c/h, section update
>
> :::::: TO: Michal Simek <monstr@monstr.eu>
> :::::: CC: Michal Simek <monstr@monstr.eu>
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

-- 
Jani Nikula, Intel Open Source Graphics Center