| Message ID | 20190327184531.30986-6-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Add support to build with clang | expand |
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@arm.com> > --- > 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? 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. ~Andrew
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@arm.com> >> --- >> 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. Cheers, -- Julien Grall
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@arm.com> > >> --- > >> 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.
Hi, 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@arm.com> >>>> --- >>>> 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. Cheers,
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@arm.com> > > > > > --- > > > > > 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);
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@arm.com> >>>>>> --- >>>>>> 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,
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@arm.com> > > > > > > > --- > > > > > > > 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
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)); + return BITS_PER_LONG - ret; }
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@arm.com> --- xen/include/asm-arm/arm64/bitops.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)