[PATCH] powerpc: fenvinline.h refactor
Adhemerval Zanella
adhemerval.zanella@linaro.org
Mon Jan 27 18:43:00 GMT 2020
On 27/01/2020 11:05, Rogerio Alves wrote:
>
>> Subject: Re: [PATCH] powerpc: fenvinline.h refactor > Date: Thu, 23 Jan 2020 18:24:38 -0300
>> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>> To: libc-alpha@sourceware.org
>>
>>
>>
>> On 23/01/2020 15:14, Rogerio Alves wrote:
>>> Hi,
>>>
>>> Attached to this email is a patch to refactor fenvinline.h for power to include some __buitins when available. I also removed a weird asm constraint. I can't reproduce the warning described on the file and the test passed. Not sure why the put that constraint. I will appreciate any reviews.
>>
>> PS: please send the patch inline next time, it helps the review
>> process.
>>
>
> Ok. Once I include you suggestions I will send a inline patch instead.
>
>>>
>>> Regard
>>
>>> Â From dbeca754bcbc6c77b6157a2f818554db31bac2ba Mon Sep 17 00:00:00 2001
>>> From: Rogerio Alves <rcardoso@linux.ibm.com>
>>> Date: Thu, 23 Jan 2020 14:54:00 -0300
>>> Subject: [PATCH v1] powerpc: Refactor fenvinline.h
>>>
>>> This patch refactor fenviline.h replaces some statements for
>>> builtins.
>>>
>>> 2020-01-22 Rogerio A. Cardoso <rcardoso@linux.ibm.com>
>>>
>>> Â Â Â Â * sysdeps/unix/sysv/linux/powerpc/bits/fenvline.h:
>>> Â Â Â Â Replace expression by __builtin_popcount. Use
>>> Â Â Â Â __builtin_mtfsb[01] when avaliable. Remove weird i#*X
>>> Â Â Â Â asm constraint.
>>
>> No ChangeLog entry required anymore.
>>
>
> Oh. I was not aware. Last time I sent a patch here was still required. I will remove.
>
>>> ---
>>> Â sysdeps/powerpc/bits/fenvinline.h | 25 +++++++++++++++----------
>>> Â 1 file changed, 15 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/sysdeps/powerpc/bits/fenvinline.h b/sysdeps/powerpc/bits/fenvinline.h
>>> index 70689664e2..ae04ece189 100644
>>> --- a/sysdeps/powerpc/bits/fenvinline.h
>>> +++ b/sysdeps/powerpc/bits/fenvinline.h
>>> @@ -15,7 +15,7 @@
>>> Â Â Â Â You should have received a copy of the GNU Lesser General Public
>>> Â Â Â Â License along with the GNU C Library; if not, see
>>>     <https://www.gnu.org/licenses/>. */
>>> -
>>> +#include <stdio.h>
>>
>> This is most likely not required.
>>
>
> Yes. I forgot to remove that.
>
>>> Â #if defined __GNUC__ && !defined _SOFT_FLOAT && !defined __NO_FPRS__
>>>   /* Inline definitions for fegetround. */
>>> @@ -51,9 +51,16 @@
>>> Â # define fegetround() __fegetround ()
>>> Â Â # ifndef __NO_MATH_INLINES
>>> -/* The weird 'i#*X' constraints on the following suppress a gcc
>>> -  warning when __excepts is not a constant. Otherwise, they mean the
>>> -  same as just plain 'i'. */
>>
>> This is an installed header which can potentially be used with a older
>> gcc version. It is most likely an old bug fixed on newer release, so to
>> use the plain 'i' constrain you need to set it to use iff with a gcc
>> version where the warning surely does not happen.
>>
>
> Yes. I will check the minimal gcc version where the warning do not happen and set it. I am working on that here.
>
>>> +
>>> +/* Builtins to mtfsb0 and mtfsb1 was introduced on GCC 9. */
>>> +#Â if !__GNUC_PREREQ(9, 0)
>>> +
>>> +#Â Â define __builtin_mtfsb0(__b)Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> +Â Â __asm__ __volatile__ ("mtfsb0 %0" : : "i" __b)
>>> +
>>> +#Â Â define __builtin_mtfsb1(__b)Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> +Â Â __asm__ __volatile__ ("mtfsb1 %0" : : "i" __b)
>>> +#Â endif
>>
>> I don't think it is a good practice to re-define bultins, use it along with
>> an extra indirection instead. Something as:
>>
>> Â Â #Â if !__GNUC_PREREQ(9, 0)
>> Â Â #Â Â define __mtfsb0(__b) __asm__ __volatile__ ("mtfsb0 %0" : : "i" (__b))
>> Â Â #Â Â define __mtfsb1(__b) __asm__ __volatile__ ("mtfsb1 %0" : : "i" (__b))
>> Â Â #Â else
>> Â Â #Â Â define __mtfsb0(__b) __builtin_mtfsb0 (__b)
>> Â Â #Â Â define __mtfsb1(__b) __builtin_mtfsb1 (__b)
>> Â Â #Â endif
>>
>
> Nice. I'll do that. Thank you for this suggestion.
Also, __mtfsb{0,1} definition clash with ppu_intrinsics.h (which is an
installed gcc header). So use __glibc_mtfsb{0,1} instead.
>
>>> Â Â #Â if __GNUC_PREREQ(3, 4)
>>> Â @@ -63,12 +70,11 @@
>>> Â Â Â Â Â int __e = __excepts;Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> Â Â Â Â Â int __ret;Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> Â Â Â Â Â if (__builtin_constant_p (__e)Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> -Â Â Â Â Â Â Â && (__e & (__e - 1)) == 0Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> +Â Â Â Â Â Â Â && (__builtin_popcount (__e) == 1)Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>
>> Why change this code? The idea is not emit any of the check for constant
>> call with only one FE_* flag (either mtfsb0 or mtfsb1).
>>
>
> In that case the test (__e & (__e - 1)) == 0 tests if only one bit is set on __e. Since FE_* flags are in power of two (except FE_ALL_EXCEPT) it has the same effect that __builtin_popcount. I guess is more readable to use __builtin_popcount(__e) == 1. The code is still the same: not emit any of the check for constant call it only one FE_ flag is set.
My understanding it builds on the assumption that all the FE_* flags
are multiple of 2, meaning only on bit will be set and thus if the
exception to be set is not an multiple of 2 then it can not be set/unset
using mtfsb{0,1}.
This change is essentially the same, so I don't see a strong motivation
to do it.
>
>>> Â Â Â Â Â Â Â Â Â && __e != FE_INVALID)Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> Â Â Â Â Â Â Â {Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> Â Â Â Â Â if (__e != 0)Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> -Â Â Â Â Â __asm__ __volatile__ ("mtfsb1 %0"Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â : : "i#*X" (__builtin_clz (__e)));Â Â Â Â Â Â Â Â Â \
>>> +Â Â Â Â Â __builtin_mtfsb1 ((__builtin_clz (__e)));Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> Â Â Â Â Â Â Â Â Â __ret = 0;Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> Â Â Â Â Â Â Â }Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>>      else                                     \
>>> @@ -82,12 +88,11 @@
>>> Â Â Â Â Â int __e = __excepts;Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> Â Â Â Â Â int __ret;Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> Â Â Â Â Â if (__builtin_constant_p (__e)Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> -Â Â Â Â Â Â Â && (__e & (__e - 1)) == 0Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> +Â Â Â Â Â Â Â && (__builtin_popcount (__e) == 1)Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> Â Â Â Â Â Â Â Â Â && __e != FE_INVALID)Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> Â Â Â Â Â Â Â {Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> Â Â Â Â Â if (__e != 0)Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> -Â Â Â Â Â __asm__ __volatile__ ("mtfsb0 %0"Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â : : "i#*X" (__builtin_clz (__e)));Â Â Â Â Â Â Â Â Â \
>>> +Â Â Â Â Â __builtin_mtfsb0 ((__builtin_clz (__e)));Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> Â Â Â Â Â Â Â Â Â __ret = 0;Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>> Â Â Â Â Â Â Â }Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
>>>      else                                     \
>>> --Â
>>> 2.17.1
More information about the Libc-alpha
mailing list