[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 05/12] xen/arm64: bitops: Match the register size with the value size in flsl
On Thu, 18 Apr 2019, Julien Grall wrote: > Hi, > > On 18/04/2019 19:40, Stefano Stabellini wrote: > > 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. > > I find quite amusing that you suggest this when in the patch #7 you mention > the MISRA rule 10.1 which forbid assigning a value to a narrower type > (unsigned long -> int). > > Regardless that, clz operation clearly ask for a 64-bit value. So uint64_t > makes much more sense than "unsigned long". In general, I am a strong advocate > to use uintX_t when the size is known. "unsigned long" should only be used in > place you need compat between 32-bit and 64-bit code. all right, OK by me _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |