Re: [Minios-devel] [UNIKRAFT PATCH v2 6/6] arch/arm64: Implement bitops for arm64

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jia,

On 27/09/2019 07:53, Jia He wrote:
> Without this patch, compiler will report warning as follows:
>    In file included from plat/common/arm/time.c:41:0:
> include/uk/bitops.h: In function 'uk_get_count_order':
> include/uk/bitops.h:89:10: warning: implicit declaration of function
> 'ukarch_fls'; did you mean 'ukarch_ffsl'? [-Wimplicit-function-declaration]
>    order = ukarch_fls(count);
>            ^~~~~~~~~~
>            ukarch_ffsl
> include/uk/bitops.h: In function 'uk_find_last_bit':
> include/uk/bitops.h:154:18: warning: implicit declaration of function
> 'ukarch_flsl'; did you mean 'ukarch_ffsl'? [-Wimplicit-function-declaration]
>      return (bit + ukarch_flsl(mask));
>                    ^~~~~~~~~~~
>                    ukarch_ffsl
This explains why you add ukarch_fls and ukarch_flsl. But this does not explain 
why you reimplemented ukarch_ffsl with a builtin.
Cheers,

> Signed-off-by: Jia He <justin.he@xxxxxxx>
> ---
>   arch/arm/arm64/include/uk/asm/atomic.h | 55 ++++++++++++++++----------
>   1 file changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/arm64/include/uk/asm/atomic.h 
> b/arch/arm/arm64/include/uk/asm/atomic.h
> index 7ee6667..cb9b829 100644
> --- a/arch/arm/arm64/include/uk/asm/atomic.h
> +++ b/arch/arm/arm64/include/uk/asm/atomic.h
> @@ -38,33 +38,46 @@
>   #endif
>   
>   /**
> - * ukarch_ffsl - find first (lowest) set bit in word.
> + * ukarch_ffs - find first (lowest) set bit in word.
>    * @word: The word to search
>    *
>    * Undefined if no bit exists, so code should check against 0 first.
>    */
> -static inline unsigned long ukarch_ffsl(unsigned long word)
> +static inline unsigned int ukarch_ffs(unsigned int x)
>   {
> -       int clz;
> +       return __builtin_ffs(x);
> +}
>   
> -	/* xxxxx10000 = word
> -        * xxxxx01111 = word - 1
> -        * 0000011111 = word ^ (word - 1)
> -        *      4     = 63 - clz(word ^ (word - 1))
> -        */
> +/**
> + * ukarch_fls - find last (highest) set bit in word.
> + * @word: The word to search
> + *
> + * Undefined if no bit exists, so code should check against 0 first.
Here you say that fls(0) is undefined but...

> + * Note fls(0) = 0, fls(1) = 1, fls(0x80000000) = 32.
... here you define fls(0).

Your code seems to implement the later behavior. Furthermore, the x86 code is 
not able to deal with fls(0). So is there any reason to allow it for Arm?
> + */
> +static inline unsigned int ukarch_fls(unsigned int x)
> +{
> +       return x ? sizeof(x) * 8 - __builtin_clz(x) : 0;
> +}
>   
> -	__asm__("sub x0, %[word], #1\n"
> -               "eor x0, x0, %[word]\n"
> -               "clz %[clz], x0\n"
> -               :
> -               /* Outputs: */
> -               [clz] "=r"(clz)
> -               :
> -               /* Inputs: */
> -               [word] "r"(word)
> -               :
> -               /* Clobbers: */
> -               "x0");
> +/**
> + * ukarch_ffsl - find first (lowest) set bit in word.
> + * @word: The word to search
> + *
> + * Undefined if no bit exists, so code should check against 0 first.
> + */
> +static inline unsigned long ukarch_ffsl(unsigned long x)
> +{
> +       return __builtin_ffsl(x);
> +}
>   
> -	return 63 - clz;
> +/**
> + * ukarch_flsl - find last (highest) set bit in word.
> + * @word: The word to search
> + * > + * Undefined if no bit exists, so code should check against 0 first.
Similar remark here.

> + */
> +static inline unsigned long ukarch_flsl(unsigned long x)
> +{
> +       return x ? sizeof(x) * 8 - __builtin_clzl(x) : 0;
>   }
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel