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

["Justin He (Arm Technology China)" <Justin.He@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 2019年9月27日 17:42
> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> Cc: Sharan.Santhanam@xxxxxxxxx; Felipe Huici <felipe.huici@xxxxxxxxx>;
> Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>;
> Santiago.Pagani@xxxxxxxxx
> Subject: Re: [UNIKRAFT PATCH v2 6/6] arch/arm64: Implement bitops for
> arm64
>
> 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.
>
Okay, I will post my test result.
I compared the two implementation (gcc builtin and ukarch_ffsl original one )

for (x=0; x<0xfffffff; x++)

original:
real    0m1.723s
user    0m1.723s
sys     0m0.000s

gcc builtin:
real    0m1.550s
user    0m1.546s
sys     0m0.004s

> 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?
>
Ok, I will keep the same page with X86's behavior.

--
Cheers,
Justin (Jia He)


> > + */
> > +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
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel