[PATCH] rs6000: Check -+0 and NaN for smax/smin generation

[PATCH] rs6000: Check -+0 and NaN for smax/smin generation

[Jiufu Guo]

Hi,

PR93709 mentioned regressions on maxlocval_4.f90 and minlocval_f.f90 which
relates to max of '-inf' and 'nan'. This regression occur on P9 which has
new instruction 'xsmaxcdp/xsmincdp'.
The similar issue also could be find on `a < b ? b : a` which is also
generated as `xsmaxcdp` under -O2 for P9. This instruction `xsmaxcdp`
more like C/C++ semantic (a>b?a:b). A testcase is added for this issue.

The following patch improve code to check -+0 and NaN before 'smax/smin' to
be generated for those cases.

Bootstrap/regtest on powerpc64le pass without regressions.
Is this OK for trunk and backport to GCC 9?

BR.
Jiufu

gcc/
2020-03-05  Jiufu Guo  <guojiufu@linux.ibm.com>

	PR target/93709
	* gcc/config/rs6000/rs6000.c (rs6000_emit_p9_fp_minmax): Check
	-fno-signed-zeros and -ffinite_math_only for smax/smin.

gcc/testsuite
2020-03-05  Jiufu Guo  <guojiufu@linux.ibm.com>

	PR target/93709
	* gcc.target/powerpc/p9-minmax-3.c: New test.

---
 gcc/config/rs6000/rs6000.c                     |  8 +++++++-
 gcc/testsuite/gcc.target/powerpc/p9-minmax-3.c | 17 +++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/p9-minmax-3.c

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index f34e1ba70c6..951a6c32884 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -14836,7 +14836,13 @@ rs6000_emit_p9_fp_minmax (rtx dest, rtx op, rtx true_cond, rtx false_cond)
   if (rtx_equal_p (op0, true_cond) && rtx_equal_p (op1, false_cond))
     ;
 
-  else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0, false_cond))
+  /* Only when -fno-signed-zeros and -ffinite_math_only are in effect,
+     `op0 < op1 ? op1 : op0` works like `op1 > op0 ? op1 : op0` which 
+     could use smax;
+     `op0 > op1 ? op1 : op0` works like `op1 < op0 ? op1 : op0` which
+     could use smin.  */
+  else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0, false_cond)
+	   && (flag_finite_math_only && !flag_signed_zeros))
     max_p = !max_p;
 
   else
diff --git a/gcc/testsuite/gcc.target/powerpc/p9-minmax-3.c b/gcc/testsuite/gcc.target/powerpc/p9-minmax-3.c
new file mode 100644
index 00000000000..141603e05b4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/p9-minmax-3.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-require-effective-target powerpc_p9vector_ok } */
+/* { dg-options "-mdejagnu-cpu=power9 -O2 -mpower9-minmax" } */
+/* { dg-final { scan-assembler-not "xsmaxcdp"   } } */
+/* { dg-final { scan-assembler-not "xsmincdp"   } } */
+
+double
+dbl_max1 (double a, double b)
+{
+  return a < b ? b : a;
+}
+
+double
+dbl_min1 (double a, double b)
+{
+  return a > b ? b : a;
+}
-- 
2.17.1

Re: [PATCH] rs6000: Check -+0 and NaN for smax/smin generation

[Segher Boessenkool]

Hi!

On Thu, Mar 05, 2020 at 10:46:58AM +0800, Jiufu Guo wrote:
> PR93709 mentioned regressions on maxlocval_4.f90 and minlocval_f.f90 which
> relates to max of '-inf' and 'nan'. This regression occur on P9 which has
> new instruction 'xsmaxcdp/xsmincdp'.
> The similar issue also could be find on `a < b ? b : a` which is also
> generated as `xsmaxcdp` under -O2 for P9. This instruction `xsmaxcdp`
> more like C/C++ semantic (a>b?a:b). A testcase is added for this issue.
> 
> The following patch improve code to check -+0 and NaN before 'smax/smin' to
> be generated for those cases.

> -  else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0, false_cond))
> +  /* Only when -fno-signed-zeros and -ffinite_math_only are in effect,
> +     `op0 < op1 ? op1 : op0` works like `op1 > op0 ? op1 : op0` which 
> +     could use smax;
> +     `op0 > op1 ? op1 : op0` works like `op1 < op0 ? op1 : op0` which
> +     could use smin.  */
> +  else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0, false_cond)
> +	   && (flag_finite_math_only && !flag_signed_zeros))
>      max_p = !max_p;

I know I asked for it, but should this use HONOR_NANS (compare_mode)
instead?  Infinities will work fine?  Just NaNs and zeros won't.

Okay for trunk with that change (if it works :-) )  Thanks!


Segher

Re: [PATCH] rs6000: Check -+0 and NaN for smax/smin generation

[Jiufu Guo]

Segher Boessenkool <segher@kernel.crashing.org> writes:

> Hi!
>
> On Thu, Mar 05, 2020 at 10:46:58AM +0800, Jiufu Guo wrote:
>> PR93709 mentioned regressions on maxlocval_4.f90 and minlocval_f.f90 which
>> relates to max of '-inf' and 'nan'. This regression occur on P9 which has
>> new instruction 'xsmaxcdp/xsmincdp'.
>> The similar issue also could be find on `a < b ? b : a` which is also
>> generated as `xsmaxcdp` under -O2 for P9. This instruction `xsmaxcdp`
>> more like C/C++ semantic (a>b?a:b). A testcase is added for this issue.
>> 
>> The following patch improve code to check -+0 and NaN before 'smax/smin' to
>> be generated for those cases.
>
>> -  else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0, false_cond))
>> +  /* Only when -fno-signed-zeros and -ffinite_math_only are in effect,
>> +     `op0 < op1 ? op1 : op0` works like `op1 > op0 ? op1 : op0` which 
>> +     could use smax;
>> +     `op0 > op1 ? op1 : op0` works like `op1 < op0 ? op1 : op0` which
>> +     could use smin.  */
>> +  else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0, false_cond)
>> +	   && (flag_finite_math_only && !flag_signed_zeros))
>>      max_p = !max_p;
>
> I know I asked for it, but should this use HONOR_NANS (compare_mode)
> instead?  Infinities will work fine?  Just NaNs and zeros won't.
HONOR_NANS(mode) is `MODE_HAS_NANS (mode) && !flag_finite_math_only`.
We know the mode is SF or DF.  Both maybe ok for current code.

I agree with you HONOR_NANS would be better, it is more generic for
front-end, gimple and rtl.  And rs6000_emit_p9_fp_minmax maybe called
without checking mode in future code, in this case HONOR_NANS is
better.

I updated the code as:
```
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index f34e1ba70c6..b057f689b56 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -14836,7 +14836,11 @@ rs6000_emit_p9_fp_minmax (rtx dest, rtx op, rtx
true_cond, rtx false_cond)
   if (rtx_equal_p (op0, true_cond) && rtx_equal_p (op1, false_cond))
        ;

-  else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0,
   false_cond))
   +  /* Only when NaNs and signed-zeros are not in effect, smax could be
   +     used for `op0 < op1 ? op1 : op0`, and smin could be used for
   +     `op0 > op1 ? op1 : op0`.  */
   +  else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0, false_cond)
   +          && !HONOR_NANS (compare_mode) &&
   !HONOR_SIGNED_ZEROS(compare_mode))
        max_p = !max_p;

   else
```
This code works fine. I'm going to submit it.

Thanks!
Jiufu Guo

>
> Okay for trunk with that change (if it works :-) )  Thanks!
>
>
> Segher

Re: [PATCH] rs6000: Check -+0 and NaN for smax/smin generation

[Jiufu Guo]

Jiufu Guo <guojiufu@linux.ibm.com> writes:

Backported to GCC 9, preapproved by Segher.

Thanks,

Jiufu

> Segher Boessenkool <segher@kernel.crashing.org> writes:
>
>> Hi!
>>
>> On Thu, Mar 05, 2020 at 10:46:58AM +0800, Jiufu Guo wrote:
>>> PR93709 mentioned regressions on maxlocval_4.f90 and minlocval_f.f90 which
>>> relates to max of '-inf' and 'nan'. This regression occur on P9 which has
>>> new instruction 'xsmaxcdp/xsmincdp'.
>>> The similar issue also could be find on `a < b ? b : a` which is also
>>> generated as `xsmaxcdp` under -O2 for P9. This instruction `xsmaxcdp`
>>> more like C/C++ semantic (a>b?a:b). A testcase is added for this issue.
>>> 
>>> The following patch improve code to check -+0 and NaN before 'smax/smin' to
>>> be generated for those cases.
>>
>>> -  else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0, false_cond))
>>> +  /* Only when -fno-signed-zeros and -ffinite_math_only are in effect,
>>> +     `op0 < op1 ? op1 : op0` works like `op1 > op0 ? op1 : op0` which 
>>> +     could use smax;
>>> +     `op0 > op1 ? op1 : op0` works like `op1 < op0 ? op1 : op0` which
>>> +     could use smin.  */
>>> +  else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0, false_cond)
>>> +	   && (flag_finite_math_only && !flag_signed_zeros))
>>>      max_p = !max_p;
>>
>> I know I asked for it, but should this use HONOR_NANS (compare_mode)
>> instead?  Infinities will work fine?  Just NaNs and zeros won't.
> HONOR_NANS(mode) is `MODE_HAS_NANS (mode) && !flag_finite_math_only`.
> We know the mode is SF or DF.  Both maybe ok for current code.
>
> I agree with you HONOR_NANS would be better, it is more generic for
> front-end, gimple and rtl.  And rs6000_emit_p9_fp_minmax maybe called
> without checking mode in future code, in this case HONOR_NANS is
> better.
>
> I updated the code as:
> ```
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index f34e1ba70c6..b057f689b56 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -14836,7 +14836,11 @@ rs6000_emit_p9_fp_minmax (rtx dest, rtx op, rtx
> true_cond, rtx false_cond)
>    if (rtx_equal_p (op0, true_cond) && rtx_equal_p (op1, false_cond))
>         ;
>
> -  else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0,
>    false_cond))
>    +  /* Only when NaNs and signed-zeros are not in effect, smax could be
>    +     used for `op0 < op1 ? op1 : op0`, and smin could be used for
>    +     `op0 > op1 ? op1 : op0`.  */
>    +  else if (rtx_equal_p (op1, true_cond) && rtx_equal_p (op0, false_cond)
>    +          && !HONOR_NANS (compare_mode) &&
>    !HONOR_SIGNED_ZEROS(compare_mode))
>         max_p = !max_p;
>
>    else
> ```
> This code works fine. I'm going to submit it.
>
> Thanks!
> Jiufu Guo
>
>>
>> Okay for trunk with that change (if it works :-) )  Thanks!
>>
>>
>> Segher

Re: [PATCH] rs6000: Check -+0 and NaN for smax/smin generation

[Matthias Klose]

On 3/19/20 7:22 AM, Jiufu Guo via Gcc-patches wrote:
> Jiufu Guo <guojiufu@linux.ibm.com> writes:
> 
> Backported to GCC 9, preapproved by Segher.
> 
> Thanks,
> 
> Jiufu

this checks in a file

diff --git a/a b/a
new file mode 100644
index 00000000000..a4f422403ef
--- /dev/null
+++ b/a
@@ -0,0 +1,81 @@
+commit 5b075372b47b87bde46e5acc58531c410fb65f8c
+Author:     Jiufu Guo <guojiufu@linux.ibm.com>
+AuthorDate: Tue Mar 10 13:51:57 2020 +0800
+Commit:     Jiufu Guo <guojiufu@linux.ibm.com>
+CommitDate: Thu Mar 19 10:04:00 2020 +0800

[...]

please remove.

Re: [PATCH] rs6000: Check -+0 and NaN for smax/smin generation

[Jiufu Guo]

Matthias Klose <doko@debian.org> writes:

Thanks so much for all of you for pay attention and take care of
this.  Matthias and Segher point out this; Joseph helped remove this
file.  Sorry for spend your extra time on this.

Thanks again!

> diff --git a/a b/a
> new file mode 100644
> index 00000000000..a4f422403ef
> --- /dev/null
> +++ b/a
> @@ -0,0 +1,81 @@
> +commit 5b075372b47b87bde46e5acc58531c410fb65f8c
> +Author:     Jiufu Guo <guojiufu@linux.ibm.com>
> +AuthorDate: Tue Mar 10 13:51:57 2020 +0800
> +Commit:     Jiufu Guo <guojiufu@linux.ibm.com>
> +CommitDate: Thu Mar 19 10:04:00 2020 +0800
>
> [...]
>
> please remove.