Re: [Xen-devel] [PATCH 05/12] xen/arm64: bitops: Match the register size with the value size in flsl

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 27 Mar 2019, Julien Grall wrote:
> Hi,
> 
> On 27/03/2019 19:03, Andrew Cooper wrote:
> > On 27/03/2019 18:45, Julien Grall wrote:
> >> Clang is pickier than GCC for the register size in asm statement. It 
> >> expects
> >> the register size to match the value size.
> >>
> >> The instruction clz is expecting the two operands to be the same size
> >> (i.e 32-bit or 64-bit). As the flsl function is dealing with 64-bit
> >> value, we need to make the destination variable 64-bit as well.
> >>
> >> While at it, add a newline before the return statement.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> >> ---
> >>   xen/include/asm-arm/arm64/bitops.h | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/include/asm-arm/arm64/bitops.h 
> >> b/xen/include/asm-arm/arm64/bitops.h
> >> index 6bf1922680..05045f1109 100644
> >> --- a/xen/include/asm-arm/arm64/bitops.h
> >> +++ b/xen/include/asm-arm/arm64/bitops.h
> >> @@ -34,12 +34,13 @@ static /*__*/always_inline unsigned long 
> >> __ffs(unsigned long word)
> >>   
> >>   static inline int flsl(unsigned long x)
> >>   {
> >> -        int ret;
> >> +        uint64_t ret;
> >>   
> >>           if (__builtin_constant_p(x))
> >>                  return generic_flsl(x);
> >>   
> >>           asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
> > 
> > As x is fixed by the ABI, ret should be unsigned long to match, surely?
> 
> No need for it. The result of the instruction clz will always be smaller 
> than 64.
> 
> I suspect they require a 64-bit register just for simplicity as they 
> encode the size for the 2 registers in only a single bit (i.e sf).
> 
> > 
> > This will compile as it is arm64 specific, but it looks just as wrong
> > (per the commit message) as using int in the first place.
> See above. I can clarify it in the commit message.

I agree with Andrew: I can see that the code is correct, but it really
looks wrong at first sight. Why not use unsigned long? This is an arm64
header anyway.

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