Re: Ping ^ 2: [PATCH] rs6000: Expand fmod and remainder when built with fast-math [PR97142]

[Xionghu Luo <luoxhu@linux.ibm.com>]

On 2021/9/4 05:44, Segher Boessenkool wrote:
> Hi!
> 
> On Fri, Sep 03, 2021 at 10:31:24AM +0800, Xionghu Luo wrote:
>> fmod/fmodf and remainder/remainderf could be expanded instead of library
>> call when fast-math build, which is much faster.
> 
> Thank you very much for this patch.
> 
> Some trivial comments if you haven't commmitted it yet:
> 
>> +(define_expand "fmod<mode>3"
>> +  [(use (match_operand:SFDF 0 "gpc_reg_operand"))
>> +	(use (match_operand:SFDF 1 "gpc_reg_operand"))
>> +	(use (match_operand:SFDF 2 "gpc_reg_operand"))]
>> +  "TARGET_HARD_FLOAT
>> +  && TARGET_FPRND
>> +  && flag_unsafe_math_optimizations"
> 
> It should have one extra space before each && here:

OK.

> 
>    "TARGET_HARD_FLOAT
>     && TARGET_FPRND
>     && flag_unsafe_math_optimizations"
> 
> (so that everything inside of the string aligns).
> 
>> +(define_expand "remainder<mode>3"
> 
> (same here).
> 
>> +/* { dg-final { scan-assembler-not {\mbl fmod\M} } } */
>> +/* { dg-final { scan-assembler-not {\mbl fmodf\M} } } */
>> +/* { dg-final { scan-assembler-not {\mbl remainder\M} } } */
>> +/* { dg-final { scan-assembler-not {\mbl remainderf\M} } } */
> 
> These are negative tests, so won't spuriously fail, but this does not
> test for the function prefixes we can have.  See
> gcc.target/powerpc/builtins-1.c for example.

Thanks.  Verified that different calls are generated on different platforms
without this patch.

P8BE-64: bl __fmodf_finite
P8BE-32: b __fmodf_finite
P8LE-64:  bl fmodf

"l", "__" and "_finite" are optional, so is it OK to check them with below patterns?

+/* { dg-final { scan-assembler-not {\mbl? (__)?fmod(_finite)?\M} } } */
+/* { dg-final { scan-assembler-not {\mbl? (__)?fmodf(_finite)?\M} } } */
+/* { dg-final { scan-assembler-not {\mbl? (__)?remainder(_finite)?\M} } } */
+/* { dg-final { scan-assembler-not {\mbl? (__)?remainderf(_finite)?\M} } } */


> 
> Again, thank you, and thanks to everyone else for the patch review
> action :-)
> 
> 
> Segher
> 

-- 
Thanks,
Xionghu