Thanks Maciej!
On Tue, May 10, 2022 at 10:05 PM Maciej W. Rozycki <ma...@embecosm.com> wrote:
>
> As at r2.2 of the RISC-V ISA specification[1] (equivalent to version 2.0
> of the "F" and "D" standard architecture extensions for single-precision
> and double-precision floating-point respectively) the FMIN and FMAX
> machine instructions fully match our requirement for the `fminM3' and
> `fmaxM3' standard RTL patterns:
>
> "For FMIN and FMAX, if at least one input is a signaling NaN, or if both
> inputs are quiet NaNs, the result is the canonical NaN. If one operand
> is a quiet NaN and the other is not a NaN, the result is the non-NaN
> operand."
>
> suitably for the IEEE 754-2008 `minNum' and `maxNum' operations.
>
> However we only define `sminM3' and `smaxM3' standard RTL patterns to
> produce the FMIN and FMAX machine instructions, which in turn causes the
> `__builtin_fmin' and `__builtin_fmax' family of intrinsics to emit the
> corresponding libcalls rather than the relevant machine instructions.
> This is according to earlier revisions of the RISC-V ISA specification,
> which we however do not support anymore, as from commit 4b81528241ca
> ("RISC-V: Support version controling for ISA standard extensions").
>
> As from r20190608 of the RISC-V ISA specification (equivalent to version
> 2.2 of the "F" and "D" standard ISA extensions for single-precision and
> double-precision floating-point respectively) the definition of the FMIN
> and FMAX machine instructions has been updated[2]:
>
> "Defined the signed-zero behavior of FMIN.fmt and FMAX.fmt, and changed
> their behavior on signaling-NaN inputs to conform to the minimumNumber
> and maximumNumber operations in the proposed IEEE 754-201x
> specification."
>
> and specifically[3]:
>
> "Floating-point minimum-number and maximum-number instructions FMIN.S
> and FMAX.S write, respectively, the smaller or larger of rs1 and rs2 to
> rd. For the purposes of these instructions only, the value -0.0 is
> considered to be less than the value +0.0. If both inputs are NaNs, the
> result is the canonical NaN. If only one operand is a NaN, the result
> is the non-NaN operand. Signaling NaN inputs set the invalid operation
> exception flag, even when the result is not NaN."
>
> Consequently for forwards compatibility with r20190608+ hardware we
> cannot use the FMIN and FMAX machine instructions unconditionally even
> where the ISA level of r2.2 has been specified with the `-misa-spec=2.2'
> option where operation would be different between ISA revisions, that
> is the handling of signaling NaN inputs.
>
> Therefore provide new `fmin<mode>3' and `fmax<mode>3' patterns removing
> the need to emit libcalls with the `__builtin_fmin' and `__builtin_fmax'
> family of intrinsics, however limit them to where `-fno-signaling-nans'
> is in effect, deferring to other code generation strategies otherwise as
> applicable. Use newly-defined UNSPECs as the operation codes so that
> the patterns are only ever used if referred to by their names, as there
> is no RTL expression defined for the IEEE 754-2008 `minNum' and `maxNum'
> operations.
>
> References:
>
> [1] "The RISC-V Instruction Set Manual, Volume I: User-Level ISA",
> Document Version 2.2, May 7, 2017, Section 8.3 "NaN Generation and
> Propagation", p. 48
>
> [1] "The RISC-V Instruction Set Manual, Volume I: Unprivileged ISA",
> Document Version 20190608-Base-Ratified, June 8, 2019, "Preface",
> p. ii
>
> [2] same, Section 11.6 "Single-Precision Floating-Point Computational
> Instructions", p. 66
>
> gcc/
> * config/riscv/riscv.md (UNSPEC_FMIN, UNSPEC_FMAX): New
> constants.
> (fmin<mode>3, fmax<mode>3): New insns.
>
> gcc/testsuite/
> * gcc.target/riscv/fmax-snan.c: New test.
> * gcc.target/riscv/fmax.c: New test.
> * gcc.target/riscv/fmaxf-snan.c: New test.
> * gcc.target/riscv/fmaxf.c: New test.
> * gcc.target/riscv/fmin-snan.c: New test.
> * gcc.target/riscv/fmin.c: New test.
> * gcc.target/riscv/fminf-snan.c: New test.
> * gcc.target/riscv/fminf.c: New test.
> * gcc.target/riscv/smax-ieee.c: New test.
> * gcc.target/riscv/smax.c: New test.
> * gcc.target/riscv/smaxf-ieee.c: New test.
> * gcc.target/riscv/smaxf.c: New test.
> * gcc.target/riscv/smin-ieee.c: New test.
> * gcc.target/riscv/smin.c: New test.
> * gcc.target/riscv/sminf-ieee.c: New test.
> * gcc.target/riscv/sminf.c: New test.
> ---
> On Mon, 28 Feb 2022, Jim Wilson wrote:
>
> > On Tue, Feb 8, 2022 at 4:35 AM Maciej W. Rozycki <ma...@embecosm.com> wrote:
> >
> > > gcc/
> > > * config/riscv/riscv.md (UNSPEC_FMIN, UNSPEC_FMAX): New
> > > constants.
> > > (fmin<mode>3, fmax<mode>3): New insns.
> > > ...
> >
> >
> > I tried testing on some of the hardware I have. Both the HiFive Unleashed
> > (2018) and HiFive Unmatched (2021) implement the current definition of
> > fmin/fmax. But the Allwinner Nezha (2021) implements the previous
> > definition of fmin/fmax. SiFive was involved with the fmin/fmax change, so
> > it isn't surprising that they implemented the new semantics before other
> > companies. The Nezha board with the T-Head C906 is a popular one, so we do
> > need to continue to support the 2017 spec, which your patch does with the
> > HONORS_SNAN checks. I agree that we don't need to worry about spec
> > versions older than that.
> >
> > This looks OK to me.
> >
> > I've got builds running in parallel on the Unleashed and Unmatched to test
> > but that will take a couple of days and I don't expect any problems since
> > you already tested it. I could do a build on the Nezha if I had to, but
> > that would take at least a week as it is a much slower board and I'd rather
> > not do that unless I have to. This is hardware implementing the older spec
> > that you probably haven't tested though.
>
> Correct, I have only verified this change with an Unmatched system.
>
> Since Stage 1 is now back I have rerun regression testing with the change
> included, both without and with `-fsignaling-nans' given. There were no
> changes in results between the runs except for intermittent failures with
> gfortran.dg/ISO_Fortran_binding_17.f90 at various optimisation levels,
> clearly unrelated as this test does some DWARF information inspection.
>
> I have committed this change now, thank you for your review.
>
> Maciej
>
> Changes from v3:
>
> - Quote `\' characters in `scan-assembler' for literal `.' in regexp.
>
> - Mechanically update ChangeLog to have its entries separately for the
> testsuite.
>
> Changes from v2:
>
> - Use UNSPECs for the `fmin'/`fmax' patterns, so that they cannot be
> matched by the RTL expression.
>
> - Revert the `HONOR_SNANS (<MODE>mode)' restriction for the `smin'/`smax'
> patterns; it was wrong, because HONOR_SNANS implies HONOR_NANS, and that
> is not true for `-ffinite-math-only' where `smin'/`smax' still have to
> work.
>
> - Add test cases.
>
> - Further clarify ISA/extension revisions/versions.
>
> Changes from v1:
>
> - Adjust heading from "RISC-V: Replace `smin'/`smax' RTL patterns with
> `fmin'/`fmax'".
>
> - Do not remove `smin'/`smax' patterns; instead make them available if
> `HONOR_SNANS (<MODE>mode)'.
>
> - Make `fmin'/`fmax' patterns available if `!HONOR_SNANS (<MODE>mode)'
> only.
>
> - Update description accordingly; refer r2.2 and r20190608 ISA specs.
> ---
> gcc/config/riscv/riscv.md | 22 ++++++++++++++++++++++
> gcc/testsuite/gcc.target/riscv/fmax-snan.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/fmax.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/fmaxf-snan.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/fmaxf.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/fmin-snan.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/fmin.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/fminf-snan.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/fminf.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/smax-ieee.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/smax.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/smaxf-ieee.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/smaxf.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/smin-ieee.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/smin.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/sminf-ieee.c | 12 ++++++++++++
> gcc/testsuite/gcc.target/riscv/sminf.c | 12 ++++++++++++
> 17 files changed, 214 insertions(+)
>
> gcc-riscv-fmin-fmax.diff
> Index: gcc/gcc/config/riscv/riscv.md
> ===================================================================
> --- gcc.orig/gcc/config/riscv/riscv.md
> +++ gcc/gcc/config/riscv/riscv.md
> @@ -42,6 +42,8 @@
> UNSPEC_COPYSIGN
> UNSPEC_LRINT
> UNSPEC_LROUND
> + UNSPEC_FMIN
> + UNSPEC_FMAX
>
> ;; Stack tie
> UNSPEC_TIE
> @@ -1216,6 +1218,26 @@
> ;;
> ;; ....................
>
> +(define_insn "fmin<mode>3"
> + [(set (match_operand:ANYF 0 "register_operand" "=f")
> + (unspec:ANYF [(use (match_operand:ANYF 1 "register_operand" " f"))
> + (use (match_operand:ANYF 2 "register_operand" " f"))]
> + UNSPEC_FMIN))]
> + "TARGET_HARD_FLOAT && !HONOR_SNANS (<MODE>mode)"
> + "fmin.<fmt>\t%0,%1,%2"
> + [(set_attr "type" "fmove")
> + (set_attr "mode" "<UNITMODE>")])
> +
> +(define_insn "fmax<mode>3"
> + [(set (match_operand:ANYF 0 "register_operand" "=f")
> + (unspec:ANYF [(use (match_operand:ANYF 1 "register_operand" " f"))
> + (use (match_operand:ANYF 2 "register_operand" " f"))]
> + UNSPEC_FMAX))]
> + "TARGET_HARD_FLOAT && !HONOR_SNANS (<MODE>mode)"
> + "fmax.<fmt>\t%0,%1,%2"
> + [(set_attr "type" "fmove")
> + (set_attr "mode" "<UNITMODE>")])
> +
> (define_insn "smin<mode>3"
> [(set (match_operand:ANYF 0 "register_operand" "=f")
> (smin:ANYF (match_operand:ANYF 1 "register_operand" " f")
> Index: gcc/gcc/testsuite/gcc.target/riscv/fmax-snan.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/fmax-snan.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fno-finite-math-only -fsigned-zeros -fsignaling-nans -dp"
> } */
> +
> +double
> +fmax (double x, double y)
> +{
> + return __builtin_fmax (x, y);
> +}
> +
> +/* { dg-final { scan-assembler-not "\tfmax\\.d\t" } } */
> +/* { dg-final { scan-assembler-not "\tfge\\.d\t" } } */
> +/* { dg-final { scan-assembler "\t(call|tail)\tfmax\t" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/fmax.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/fmax.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fno-finite-math-only -fsigned-zeros -fno-signaling-nans
> -dp" } */
> +
> +double
> +fmax (double x, double y)
> +{
> + return __builtin_fmax (x, y);
> +}
> +
> +/* { dg-final { scan-assembler-not "\ttail\tfmax\t" } } */
> +/* { dg-final { scan-assembler-not "\tfge\\.d\t" } } */
> +/* { dg-final { scan-assembler "\tfmax\\.d\t\[^\n\]* fmaxdf3\n" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/fmaxf-snan.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/fmaxf-snan.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fno-finite-math-only -fsigned-zeros -fsignaling-nans -dp"
> } */
> +
> +float
> +fmaxf (float x, float y)
> +{
> + return __builtin_fmaxf (x, y);
> +}
> +
> +/* { dg-final { scan-assembler-not "\tfmax\\.s\t" } } */
> +/* { dg-final { scan-assembler-not "\tfge\\.s\t" } } */
> +/* { dg-final { scan-assembler "\t(call|tail)\tfmaxf\t" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/fmaxf.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/fmaxf.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fno-finite-math-only -fsigned-zeros -fno-signaling-nans
> -dp" } */
> +
> +float
> +fmaxf (float x, float y)
> +{
> + return __builtin_fmaxf (x, y);
> +}
> +
> +/* { dg-final { scan-assembler-not "\ttail\tfmaxf\t" } } */
> +/* { dg-final { scan-assembler-not "\tfge\\.s\t" } } */
> +/* { dg-final { scan-assembler "\tfmax\\.s\t\[^\n\]* fmaxsf3\n" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/fmin-snan.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/fmin-snan.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fno-finite-math-only -fsigned-zeros -fsignaling-nans -dp"
> } */
> +
> +double
> +fmin (double x, double y)
> +{
> + return __builtin_fmin (x, y);
> +}
> +
> +/* { dg-final { scan-assembler-not "\tfmin\\.d\t" } } */
> +/* { dg-final { scan-assembler-not "\tfle\\.d\t" } } */
> +/* { dg-final { scan-assembler "\t(call|tail)\tfmin\t" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/fmin.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/fmin.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fno-finite-math-only -fsigned-zeros -fno-signaling-nans
> -dp" } */
> +
> +double
> +fmin (double x, double y)
> +{
> + return __builtin_fmin (x, y);
> +}
> +
> +/* { dg-final { scan-assembler-not "\ttail\tfmin\t" } } */
> +/* { dg-final { scan-assembler-not "\tfle\\.d\t" } } */
> +/* { dg-final { scan-assembler "\tfmin\\.d\t\[^\n\]* fmindf3\n" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/fminf-snan.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/fminf-snan.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fno-finite-math-only -fsigned-zeros -fsignaling-nans -dp"
> } */
> +
> +float
> +fminf (float x, float y)
> +{
> + return __builtin_fminf (x, y);
> +}
> +
> +/* { dg-final { scan-assembler-not "\tfmin\\.s\t" } } */
> +/* { dg-final { scan-assembler-not "\tfle\\.s\t" } } */
> +/* { dg-final { scan-assembler "\t(call|tail)\tfminf\t" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/fminf.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/fminf.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fno-finite-math-only -fsigned-zeros -fno-signaling-nans
> -dp" } */
> +
> +float
> +fminf (float x, float y)
> +{
> + return __builtin_fminf (x, y);
> +}
> +
> +/* { dg-final { scan-assembler-not "\ttail\tfminf\t" } } */
> +/* { dg-final { scan-assembler-not "\tfle\\.s\t" } } */
> +/* { dg-final { scan-assembler "\tfmin\\.s\t\[^\n\]* fminsf3\n" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/smax-ieee.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/smax-ieee.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-ffinite-math-only -fsigned-zeros -dp" } */
> +
> +double
> +smax (double x, double y)
> +{
> + return x >= y ? x : y;
> +}
> +
> +/* { dg-final { scan-assembler-not "\t(call|tail)\tfmax\t" } } */
> +/* { dg-final { scan-assembler-not "\tfmax\\.d\t" } } */
> +/* { dg-final { scan-assembler "\tfge\\.d\t" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/smax.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/smax.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-ffinite-math-only -fno-signed-zeros -dp" } */
> +
> +double
> +smax (double x, double y)
> +{
> + return x >= y ? x : y;
> +}
> +
> +/* { dg-final { scan-assembler-not "\ttail\tfmax\t" } } */
> +/* { dg-final { scan-assembler-not "\tfge\\.d\t" } } */
> +/* { dg-final { scan-assembler "\tfmax\\.d\t\[^\n\]* smaxdf3\n" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/smaxf-ieee.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/smaxf-ieee.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-ffinite-math-only -fsigned-zeros -dp" } */
> +
> +float
> +smaxf (float x, float y)
> +{
> + return x >= y ? x : y;
> +}
> +
> +/* { dg-final { scan-assembler-not "\t(call|tail)\tfmaxf\t" } } */
> +/* { dg-final { scan-assembler-not "\tfmax\\.s\t" } } */
> +/* { dg-final { scan-assembler "\tfge\\.s\t" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/smaxf.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/smaxf.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-ffinite-math-only -fno-signed-zeros -dp" } */
> +
> +float
> +smaxf (float x, float y)
> +{
> + return x >= y ? x : y;
> +}
> +
> +/* { dg-final { scan-assembler-not "\ttail\tfmaxf\t" } } */
> +/* { dg-final { scan-assembler-not "\tfge\\.s\t" } } */
> +/* { dg-final { scan-assembler "\tfmax\\.s\t\[^\n\]* smaxsf3\n" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/smin-ieee.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/smin-ieee.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-ffinite-math-only -fsigned-zeros -dp" } */
> +
> +double
> +smin (double x, double y)
> +{
> + return x <= y ? x : y;
> +}
> +
> +/* { dg-final { scan-assembler-not "\t(call|tail)\tfmin\t" } } */
> +/* { dg-final { scan-assembler-not "\tfmin\\.d\t" } } */
> +/* { dg-final { scan-assembler "\tfle\\.d\t" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/smin.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/smin.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-ffinite-math-only -fno-signed-zeros -dp" } */
> +
> +double
> +smin (double x, double y)
> +{
> + return x <= y ? x : y;
> +}
> +
> +/* { dg-final { scan-assembler-not "\ttail\tfmin\t" } } */
> +/* { dg-final { scan-assembler-not "\tfle\\.d\t" } } */
> +/* { dg-final { scan-assembler "\tfmin\\.d\t\[^\n\]* smindf3\n" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/sminf-ieee.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/sminf-ieee.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-ffinite-math-only -fsigned-zeros -dp" } */
> +
> +float
> +sminf (float x, float y)
> +{
> + return x <= y ? x : y;
> +}
> +
> +/* { dg-final { scan-assembler-not "\t(call|tail)\tfminf\t" } } */
> +/* { dg-final { scan-assembler-not "\tfmin\\.s\t" } } */
> +/* { dg-final { scan-assembler "\tfle\\.s\t" } } */
> Index: gcc/gcc/testsuite/gcc.target/riscv/sminf.c
> ===================================================================
> --- /dev/null
> +++ gcc/gcc/testsuite/gcc.target/riscv/sminf.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-ffinite-math-only -fno-signed-zeros -dp" } */
> +
> +float
> +sminf (float x, float y)
> +{
> + return x <= y ? x : y;
> +}
> +
> +/* { dg-final { scan-assembler-not "\ttail\tfminf\t" } } */
> +/* { dg-final { scan-assembler-not "\tfle\\.s\t" } } */
> +/* { dg-final { scan-assembler "\tfmin\\.s\t\[^\n\]* sminsf3\n" } } */
