linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] log2: make is_power_of_2() integer constant expression when possible
@ 2019-03-01 12:52 Jani Nikula
  2019-03-01 13:07 ` Chris Wilson
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Jani Nikula @ 2019-03-01 12:52 UTC (permalink / raw)
  To: linux-kernel; +Cc: Andrew Morton, jani.nikula, Chris Wilson

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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] log2: make is_power_of_2() integer constant expression when possible
  2019-03-01 12:52 [PATCH] log2: make is_power_of_2() integer constant expression when possible Jani Nikula
@ 2019-03-01 13:07 ` Chris Wilson
  2019-03-01 20:26 ` Andrew Morton
       [not found] ` <201903041956.eLAFVhUK%fengguang.wu@intel.com>
  2 siblings, 0 replies; 5+ messages in thread
From: Chris Wilson @ 2019-03-01 13:07 UTC (permalink / raw)
  To: Jani Nikula, linux-kernel; +Cc: Andrew Morton, jani.nikula

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] log2: make is_power_of_2() integer constant expression when possible
  2019-03-01 12:52 [PATCH] log2: make is_power_of_2() integer constant expression when possible Jani Nikula
  2019-03-01 13:07 ` Chris Wilson
@ 2019-03-01 20:26 ` Andrew Morton
  2019-03-04 10:16   ` Jani Nikula
       [not found] ` <201903041956.eLAFVhUK%fengguang.wu@intel.com>
  2 siblings, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2019-03-01 20:26 UTC (permalink / raw)
  To: Jani Nikula; +Cc: linux-kernel, Chris Wilson

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?


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] log2: make is_power_of_2() integer constant expression when possible
  2019-03-01 20:26 ` Andrew Morton
@ 2019-03-04 10:16   ` Jani Nikula
  0 siblings, 0 replies; 5+ messages in thread
From: Jani Nikula @ 2019-03-04 10:16 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, Chris Wilson

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] log2: make is_power_of_2() integer constant expression when possible
       [not found] ` <201903041956.eLAFVhUK%fengguang.wu@intel.com>
@ 2019-03-04 11:47   ` Jani Nikula
  0 siblings, 0 replies; 5+ messages in thread
From: Jani Nikula @ 2019-03-04 11:47 UTC (permalink / raw)
  To: kbuild test robot; +Cc: kbuild-all, linux-kernel, Andrew Morton, Chris Wilson

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-03-04 11:45 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-01 12:52 [PATCH] log2: make is_power_of_2() integer constant expression when possible Jani Nikula
2019-03-01 13:07 ` Chris Wilson
2019-03-01 20:26 ` Andrew Morton
2019-03-04 10:16   ` Jani Nikula
     [not found] ` <201903041956.eLAFVhUK%fengguang.wu@intel.com>
2019-03-04 11:47   ` Jani Nikula

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).