All of the ffs()/fls() infrastructure is in fact (attr) const, because it
doesn't even read global state.  This allows the compiler even more
flexibility to optimise.

No functional change.

Reported-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
CC: Shawn Anastasio <sanastasio@raptorengineering.com>

v2:
 * New
---
 xen/include/xen/bitops.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
index 1cd43e464d9e..94af6da18b9b 100644
--- a/xen/include/xen/bitops.h
+++ b/xen/include/xen/bitops.h
@@ -32,8 +32,8 @@ extern void __bitop_bad_size(void);
  *
  * Bits are labelled from 1.  Returns 0 if given 0.
  */
-unsigned int __pure generic_ffsl(unsigned long x);
-unsigned int __pure generic_flsl(unsigned long x);
+unsigned int attr_const generic_ffsl(unsigned long x);
+unsigned int attr_const generic_flsl(unsigned long x);
 
 /**
  * generic__test_and_set_bit - Set a bit and return its old value
@@ -204,7 +204,7 @@ static always_inline bool test_bit(int nr, const volatile void *addr)
     test_bit(nr, addr);                                 \
 })
 
-static always_inline __pure unsigned int ffs(unsigned int x)
+static always_inline attr_const unsigned int ffs(unsigned int x)
 {
     if ( __builtin_constant_p(x) )
         return __builtin_ffs(x);
@@ -216,7 +216,7 @@ static always_inline __pure unsigned int ffs(unsigned int x)
 #endif
 }
 
-static always_inline __pure unsigned int ffsl(unsigned long x)
+static always_inline attr_const unsigned int ffsl(unsigned long x)
 {
     if ( __builtin_constant_p(x) )
         return __builtin_ffsl(x);
@@ -228,7 +228,7 @@ static always_inline __pure unsigned int ffsl(unsigned long x)
 #endif
 }
 
-static always_inline __pure unsigned int ffs64(uint64_t x)
+static always_inline attr_const unsigned int ffs64(uint64_t x)
 {
     if ( BITS_PER_LONG == 64 )
         return ffsl(x);
@@ -246,7 +246,7 @@ static always_inline __pure unsigned int ffs64(uint64_t x)
      sizeof(x) <= sizeof(uint64_t) ? ffs64(x) :         \
      ({ BUILD_ERROR("ffs_g() Bad input type"); 0; }))
 
-static always_inline __pure unsigned int fls(unsigned int x)
+static always_inline attr_const unsigned int fls(unsigned int x)
 {
     if ( __builtin_constant_p(x) )
         return x ? 32 - __builtin_clz(x) : 0;
@@ -258,7 +258,7 @@ static always_inline __pure unsigned int fls(unsigned int x)
 #endif
 }
 
-static always_inline __pure unsigned int flsl(unsigned long x)
+static always_inline attr_const unsigned int flsl(unsigned long x)
 {
     if ( __builtin_constant_p(x) )
         return x ? BITS_PER_LONG - __builtin_clzl(x) : 0;
@@ -270,7 +270,7 @@ static always_inline __pure unsigned int flsl(unsigned long x)
 #endif
 }
 
-static always_inline __pure unsigned int fls64(uint64_t x)
+static always_inline attr_const unsigned int fls64(uint64_t x)
 {
     if ( BITS_PER_LONG == 64 )
         return flsl(x);
-- 
2.39.2

On 29.08.2024 00:03, Andrew Cooper wrote:
> All of the ffs()/fls() infrastructure is in fact (attr) const, because it
> doesn't even read global state.  This allows the compiler even more
> flexibility to optimise.
> 
> No functional change.
> 
> Reported-by: Jan Beulich <JBeulich@suse.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> CC: Shawn Anastasio <sanastasio@raptorengineering.com>
> 
> v2:
>  * New
> ---
>  xen/include/xen/bitops.h | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

The various arch_* forms didn't have __pure and hence also don't gain
attr_const presumably because we deem these attributes ineffectual for
always-inline functions? On that basis
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

On 29/08/2024 3:07 pm, Jan Beulich wrote:
> On 29.08.2024 00:03, Andrew Cooper wrote:
>> All of the ffs()/fls() infrastructure is in fact (attr) const, because it
>> doesn't even read global state.  This allows the compiler even more
>> flexibility to optimise.
>>
>> No functional change.
>>
>> Reported-by: Jan Beulich <JBeulich@suse.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien@xen.org>
>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> CC: Bertrand Marquis <bertrand.marquis@arm.com>
>> CC: Michal Orzel <michal.orzel@amd.com>
>> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> CC: Shawn Anastasio <sanastasio@raptorengineering.com>
>>
>> v2:
>>  * New
>> ---
>>  xen/include/xen/bitops.h | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
> The various arch_* forms didn't have __pure and hence also don't gain
> attr_const presumably because we deem these attributes ineffectual for
> always-inline functions? On that basis
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

It's not quite that.

The non static-inline ones definitely do need the attribute.  That's the
only thing the optimiser has to operate with.

The static inlines shouldn't need it for GCC's benefit.  GCC will
apparently (according to buzilla) silently drop the attribute if it
believes it to have been erroneous.

However, Eclair does care about the attributes even on static-inlines as
part of it's side-effects analysis for various rules.

Therefore I've been putting the attributes on the APIs we expect code to
be using, but not on the arch_* infrastructure.  Whether this is the
right balance remains to be seen.

~Andrew