This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3] powerpc: Refactor fenvinline.h
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: Tulio Magno Quites Machado Filho <tuliom at ascii dot art dot br>, Rogerio Alves <rcardoso at linux dot ibm dot com>, libc-alpha at sourceware dot org
- Cc: pc at us dot ibm dot com
- Date: Tue, 25 Feb 2020 17:03:55 -0300
- Subject: Re: [PATCH v3] powerpc: Refactor fenvinline.h
- References: <20200210150902.129948-1-rcardoso@linux.ibm.com> <87pne2v84a.fsf@linux.ibm.com>
On 25/02/2020 15:23, Tulio Magno Quites Machado Filho wrote:
> Rogerio Alves <rcardoso@linux.ibm.com> writes:
>
>> /* 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'. */
>> + same as just plain 'i'. This warning only happens in old GCC
>
> 2 spaces here ---------------^ trailing whitespace here ------^
>
>> + versions (gcc 3 or less). Otherwise plain 'i' works fine. */
>
> Likewise ----------------------^
>
>> - if (__e != 0) \
>> - __asm__ __volatile__ ("mtfsb1 %0" \
>> - : : "i#*X" (__builtin_clz (__e))); \
>> - __ret = 0; \
>> + __MTFSB1 ((__builtin_clz (__e))); \
>
> Wrong indentation.
>
>> && __e != FE_INVALID) \
>> { \
>> - if (__e != 0) \
>> - __asm__ __volatile__ ("mtfsb0 %0" \
>> - : : "i#*X" (__builtin_clz (__e))); \
>> - __ret = 0; \
>> + __MTFSB0 ((__builtin_clz (__e))); \
>
> Wrong indentation.
>
>> @@ -80,15 +87,12 @@
>> # define feclearexcept(__excepts) \
>> (__extension__ ({ \
>> int __e = __excepts; \
>> - int __ret; \
>> + int __ret = 0; \
>> if (__builtin_constant_p (__e) \
>> - && (__e & (__e - 1)) == 0 \
>> + && (__builtin_popcount (__e) == 1) \
>
> I remember that Adhemerval disagreed with this particular change and I
> don't remember seeing any agreement there.
My reservation is the change is just an idiomatic one, it is not either
simplifying the code or optimizing it. I am ok with the usage of
powerof2, as Florian suggested.
> Rogerio, if you agree with the removal of these lines I can remove this change
> from the patch, fix the cosmetic issues and push the patch.
> Does it look good for you?
>