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, 17 Apr 2019, Julien Grall wrote:
> On 4/17/19 9:24 PM, Stefano Stabellini wrote:
> > 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.
> 
> While this is implemented in arm64, this is a function used in common code...
> So the prototype would have to be changed everywhere.
> 
> However, as I pointed out this is a design decision from Arm64 and I think it
> would be wrong to return unsigned long.
> 
> I offered the suggestion to update the commit message. I can also add a
> comment in the code.

Reading your reply, I am pretty sure that we are talking about different
things. I agree that we don't want to change the prototype everywhere,
arm32, x86. It would be good to add a comment to the code or the commit
message, I leave it to you.

My suggestion was basically "code style". I only meant the below. To me
it looks nicer because at least the ret var is the same type of the
argument, and it is more immediately obvious that the clz operation is
correct. Not a big deal.

diff --git a/xen/include/asm-arm/arm64/bitops.h 
b/xen/include/asm-arm/arm64/bitops.h
index 6bf1922..44cceb3 100644
--- a/xen/include/asm-arm/arm64/bitops.h
+++ b/xen/include/asm-arm/arm64/bitops.h
@@ -34,7 +34,7 @@ static /*__*/always_inline unsigned long __ffs(unsigned long 
word)
 
 static inline int flsl(unsigned long x)
 {
-        int ret;
+        unsigned long ret;
 
         if (__builtin_constant_p(x))
                return generic_flsl(x);




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