[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
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.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
