Re: [PATCH] rs6000: Check -+0 and NaN for smax/smin generation
Matthias Klose 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 000..a4f422403ef > --- /dev/null > +++ b/a > @@ -0,0 +1,81 @@ > +commit 5b075372b47b87bde46e5acc58531c410fb65f8c > +Author: Jiufu Guo > +AuthorDate: Tue Mar 10 13:51:57 2020 +0800 > +Commit: Jiufu Guo > +CommitDate: Thu Mar 19 10:04:00 2020 +0800 > > [...] > > please remove.
Re: [PATCH] rs6000: Check -+0 and NaN for smax/smin generation
On 3/19/20 7:22 AM, Jiufu Guo via Gcc-patches wrote: > Jiufu Guo 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 000..a4f422403ef --- /dev/null +++ b/a @@ -0,0 +1,81 @@ +commit 5b075372b47b87bde46e5acc58531c410fb65f8c +Author: Jiufu Guo +AuthorDate: Tue Mar 10 13:51:57 2020 +0800 +Commit: Jiufu Guo +CommitDate: Thu Mar 19 10:04:00 2020 +0800 [...] please remove.
Re: [PATCH] rs6000: Check -+0 and NaN for smax/smin generation
Jiufu Guo writes: Backported to GCC 9, preapproved by Segher. Thanks, Jiufu > Segher Boessenkool 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
Segher Boessenkool 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
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
[PATCH] rs6000: Check -+0 and NaN for smax/smin generation
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
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
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 000..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
