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

[Julien Grall <julien.grall@xxxxxxx>]

On 27/09/2019 10:56, Justin He (Arm Technology China) wrote:
> Hi Julien
Hi,

> > -----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 )
TBH, using builtin is better as you can let the GCC folks to maintain the 
function and therefore one less for Unikraft community to take care of :).
There are a few cases (such as atomics) where this may not be the best solution 
to use builtin as you may want to further optimize it at runtime. But that's a 
different topic ;).
Anyway, I would suggest to move the rework of ukarch_ffsl in a separate patch.

> 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
If you plan to add the test result in the commit message, then please mention 
the compiler version used and also the platform for testing.
Cheers,

--
Julien Grall

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