[GCC13?] RISC-V: Replace `smin'/`smax' RTL patterns with `fmin'/`fmax'

  RISC-V FMIN and FMAX machine instructions are IEEE-754-conformant[1]:

"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."

as required by our `fminM3' and `fmaxM3' standard RTL patterns.

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.

Rename the `smin<mode>3' and `smax<mode>3' patterns to `fmin<mode>3' and 
`fmax<mode>3' respectively then, removing the need to use libcalls for 
IEEE 754 semantics with the minimum and maximum operations.

[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

	gcc/
	* config/riscv/riscv.md (smin<mode>3): Rename pattern to...
	(fmin<mode>3): ... this.
	(smax<mode>3): Likewise...
	(fmax<mode>3): ... this.
---
Hi,

 It's not clear to me how it's been missed or whether there is anything I 
might be actually missing.  It looks to me like a clear oversight however. 
And in any case this change has passed full GCC regression testing (except 
for the D frontend, which has stopped being built recently due to a defect 
in Debian I haven't yet got to getting fixed) with the `riscv64-linux-gnu' 
target using the HiFive Unmatched (U74 CPU) target board, so it seems to 
be doing the right thing.

 Timing might a bit unfortunate for this submission and given that it is 
not a regression fix I guess this is GCC 13 material.  Please let me know 
otherwise.

 In any case OK to apply (when the time comes)?

  Maciej
---
 gcc/config/riscv/riscv.md |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

gcc-riscv-fmin-fmax.diff
  

On Thu, 20 Jan 2022 07:44:25 PST (-0800), macro@embecosm.com wrote:
> RISC-V FMIN and FMAX machine instructions are IEEE-754-conformant[1]:
>
> "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."
>
> as required by our `fminM3' and `fmaxM3' standard RTL patterns.
>
> 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.
>
> Rename the `smin<mode>3' and `smax<mode>3' patterns to `fmin<mode>3' and
> `fmax<mode>3' respectively then, removing the need to use libcalls for
> IEEE 754 semantics with the minimum and maximum operations.
>
> [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
>
> 	gcc/
> 	* config/riscv/riscv.md (smin<mode>3): Rename pattern to...
> 	(fmin<mode>3): ... this.
> 	(smax<mode>3): Likewise...
> 	(fmax<mode>3): ... this.
> ---
> Hi,
>
>  It's not clear to me how it's been missed or whether there is anything I
> might be actually missing.  It looks to me like a clear oversight however.

I'm not really a floating point person, but IIUC It's actually on 
purpose: earlier versions of the ISA spec didn't have this behavior, and 
at the time we originally merged the GCC port we decided to play it 
safe.  Pretty sure we discussed this before on the GCC mailing list 
,maybe around the time the glibc port was going upstream?  I think Jim 
was the one who figured out how all the specs fit together.

I can't find those older discussions, but this definately recently came 
up in glibc:
https://sourceware.org/pipermail/libc-alpha/2021-October/131637.html .  
Looks like back then nobody knew of any hardware that ran glibc and 
implemented the old behavior, but there also haven't been patches posted 
yet so it's not set in stone.

It's probably worth repeating the question here since there are a lot of 
RISC-V users that don't use glibc but do use GCC.  I don't know of 
anyone who implemented the old floating point standards off the top of 
my head, even in embedded land, but I'm pretty lost when it comes to ISA 
versioning these days so I might be missing something.

One option could be to tie this to the ISA spec version and emit the 
required emulation routines, but I don't think that's worth bothering to 
do unless someone knows of an implementation that implements the old 
behavior.

> And in any case this change has passed full GCC regression testing (except
> for the D frontend, which has stopped being built recently due to a defect
> in Debian I haven't yet got to getting fixed) with the `riscv64-linux-gnu'
> target using the HiFive Unmatched (U74 CPU) target board, so it seems to
> be doing the right thing.
>
>  Timing might a bit unfortunate for this submission and given that it is
> not a regression fix I guess this is GCC 13 material.  Please let me know
> otherwise.
>
>  In any case OK to apply (when the time comes)?

IMO waiting is the right way to go, as if this does uncover any issues 
they'll be a long-tail sort of thing.  That way we'll at least have a 
whole release cycle for folks to test on their hardware, which is about 
as good as we can do here.

Acked-by: Palmer Dabbelt <palmer@rivosinc.com> # for 13

Someone should probably do the glibc version, too ;)

>
>   Maciej
> ---
>  gcc/config/riscv/riscv.md |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> 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
> @@ -1214,7 +1214,7 @@
>  ;;
>  ;;  ....................
>
> -(define_insn "smin<mode>3"
> +(define_insn "fmin<mode>3"
>    [(set (match_operand:ANYF            0 "register_operand" "=f")
>  	(smin:ANYF (match_operand:ANYF 1 "register_operand" " f")
>  		   (match_operand:ANYF 2 "register_operand" " f")))]
> @@ -1223,7 +1223,7 @@
>    [(set_attr "type" "fmove")
>     (set_attr "mode" "<UNITMODE>")])
>
> -(define_insn "smax<mode>3"
> +(define_insn "fmax<mode>3"
>    [(set (match_operand:ANYF            0 "register_operand" "=f")
>  	(smax:ANYF (match_operand:ANYF 1 "register_operand" " f")
>  		   (match_operand:ANYF 2 "register_operand" " f")))]
  

On Thu, 20 Jan 2022, Maciej W. Rozycki wrote:

> RISC-V FMIN and FMAX machine instructions are IEEE-754-conformant[1]:
> 
> "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."
> 
> as required by our `fminM3' and `fmaxM3' standard RTL patterns.
> 
> 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.
> 
> Rename the `smin<mode>3' and `smax<mode>3' patterns to `fmin<mode>3' and 
> `fmax<mode>3' respectively then, removing the need to use libcalls for 
> IEEE 754 semantics with the minimum and maximum operations.
> 
> [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

That's an old version of the instruction set, and an old version of IEEE 
754.

The C functions fmin and fmax correspond to the IEEE 754-2008 operations 
minNum and maxNum, which operate as described, and the RISC-V 'F' and 'D' 
extensions *before* version 2.2 of those extensions also corresponded to 
minNum and maxNum.

IEEE 754-2019 removes minNum and maxNum because they are non-associative 
in the presence of signaling NaNs, replacing them with new operations 
minimum, minimumNumber, maximum, maximumNumber.  C23 defines new functions 
fminimum, fminimum_num, fmaximum, fmaximum_num corresponding to those new 
operations, leaving fmin and fmax unchanged.  And the RISC-V 'F' and 'D' 
extensions version 2.2 change the FMIN and FMAX instructions to correspond 
to minimumNumber and maximumNumber instead of minNum and maxNum.

So, if generating code that might be run on processors implementing 
versions of 'F' and 'D' older than 2.2, it's not safe to generate FMAX and 
FMIN instructions for any of those standard C library functions when 
signaling NaN operands are a possibility (i.e. flag_signaling_nans is 
set), because code built for older versions might run on newer versions 
with changed instruction semantics.  If generating code that requires 
version 2.2 or later of 'F' and 'D' (and I don't know if GCC actually 
supports generating code for older versions), it's OK to generate those 
instructions as part of expanding calls to the C23 functions fminimum_num 
and fmaximum_num - but not as part of expanding calls to fmin and fmax 
(unless !flag_signaling_nans, in which case the differences between those 
functions aren't relevant).  And GCC currently doesn't have built-in 
functions for fminimum_num and fmaximum_num, and I don't think it has insn 
patterns corresponding to those functions either.
  

On Thu, 20 Jan 2022, Joseph Myers wrote:

> The C functions fmin and fmax correspond to the IEEE 754-2008 operations 
> minNum and maxNum, which operate as described, and the RISC-V 'F' and 'D' 
> extensions *before* version 2.2 of those extensions also corresponded to 
> minNum and maxNum.

And, to make things clear for people confused by the different version 
numbers involved: RISC-V ISA version 2.2 contains 'F' and 'D' extensions 
version 2.0.  It's version 2.2 of the extensions, *not* of the ISA, that 
changed the instructions from minNum and maxNum to minimumNumber and 
maximumNumber.
  

On Thu, Jan 20, 2022 at 12:30 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Thu, 20 Jan 2022 07:44:25 PST (-0800), macro@embecosm.com wrote:
> > RISC-V FMIN and FMAX machine instructions are IEEE-754-conformant[1]:
> >
> > "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."
> >
> > as required by our `fminM3' and `fmaxM3' standard RTL patterns.
> >
> > 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.
> >
> > Rename the `smin<mode>3' and `smax<mode>3' patterns to `fmin<mode>3' and
> > `fmax<mode>3' respectively then, removing the need to use libcalls for
> > IEEE 754 semantics with the minimum and maximum operations.
> >
> > [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
> >
> >       gcc/
> >       * config/riscv/riscv.md (smin<mode>3): Rename pattern to...
> >       (fmin<mode>3): ... this.
> >       (smax<mode>3): Likewise...
> >       (fmax<mode>3): ... this.
> > ---
> > Hi,
> >
> >  It's not clear to me how it's been missed or whether there is anything I
> > might be actually missing.  It looks to me like a clear oversight however.
>
> I'm not really a floating point person, but IIUC It's actually on
> purpose: earlier versions of the ISA spec didn't have this behavior, and
> at the time we originally merged the GCC port we decided to play it
> safe.  Pretty sure we discussed this before on the GCC mailing list
> ,maybe around the time the glibc port was going upstream?  I think Jim
> was the one who figured out how all the specs fit together.
>
> I can't find those older discussions, but this definately recently came
> up in glibc:
> https://sourceware.org/pipermail/libc-alpha/2021-October/131637.html .
> Looks like back then nobody knew of any hardware that ran glibc and
> implemented the old behavior, but there also haven't been patches posted
> yet so it's not set in stone.
>
> It's probably worth repeating the question here since there are a lot of
> RISC-V users that don't use glibc but do use GCC.  I don't know of
> anyone who implemented the old floating point standards off the top of
> my head, even in embedded land, but I'm pretty lost when it comes to ISA
> versioning these days so I might be missing something.
>
> One option could be to tie this to the ISA spec version and emit the
> required emulation routines, but I don't think that's worth bothering to
> do unless someone knows of an implementation that implements the old
> behavior.

The old formulation of the instructions were never ratified as a
RISC-V standard.  I don't think we need to hamstring ourselves here by
assuming the possibility of their implementation.

>
> > And in any case this change has passed full GCC regression testing (except
> > for the D frontend, which has stopped being built recently due to a defect
> > in Debian I haven't yet got to getting fixed) with the `riscv64-linux-gnu'
> > target using the HiFive Unmatched (U74 CPU) target board, so it seems to
> > be doing the right thing.
> >
> >  Timing might a bit unfortunate for this submission and given that it is
> > not a regression fix I guess this is GCC 13 material.  Please let me know
> > otherwise.
> >
> >  In any case OK to apply (when the time comes)?
>
> IMO waiting is the right way to go, as if this does uncover any issues
> they'll be a long-tail sort of thing.  That way we'll at least have a
> whole release cycle for folks to test on their hardware, which is about
> as good as we can do here.
>
> Acked-by: Palmer Dabbelt <palmer@rivosinc.com> # for 13
>
> Someone should probably do the glibc version, too ;)
>
> >
> >   Maciej
> > ---
> >  gcc/config/riscv/riscv.md |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > 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
> > @@ -1214,7 +1214,7 @@
> >  ;;
> >  ;;  ....................
> >
> > -(define_insn "smin<mode>3"
> > +(define_insn "fmin<mode>3"
> >    [(set (match_operand:ANYF            0 "register_operand" "=f")
> >       (smin:ANYF (match_operand:ANYF 1 "register_operand" " f")
> >                  (match_operand:ANYF 2 "register_operand" " f")))]
> > @@ -1223,7 +1223,7 @@
> >    [(set_attr "type" "fmove")
> >     (set_attr "mode" "<UNITMODE>")])
> >
> > -(define_insn "smax<mode>3"
> > +(define_insn "fmax<mode>3"
> >    [(set (match_operand:ANYF            0 "register_operand" "=f")
> >       (smax:ANYF (match_operand:ANYF 1 "register_operand" " f")
> >                  (match_operand:ANYF 2 "register_operand" " f")))]
  

On Thu, 20 Jan 2022, Andrew Waterman wrote:

> The old formulation of the instructions were never ratified as a
> RISC-V standard.  I don't think we need to hamstring ourselves here by
> assuming the possibility of their implementation.

If you ignore the old version, then the instructions can unconditionally 
be used for the fminimum_num / fmaximum_num functions (which GCC doesn't 
support as built-in functions at present) - but can't be used for the fmin 
/ fmax functions if flag_signaling_nans.
  

On Thu, 20 Jan 2022, Joseph Myers wrote:

> > The old formulation of the instructions were never ratified as a
> > RISC-V standard.  I don't think we need to hamstring ourselves here by
> > assuming the possibility of their implementation.
> 
> If you ignore the old version, then the instructions can unconditionally 
> be used for the fminimum_num / fmaximum_num functions (which GCC doesn't 
> support as built-in functions at present) - but can't be used for the fmin 
> / fmax functions if flag_signaling_nans.

 Fair enough, thank you for your detailed explanation of the requirements 
set by the individual revisions of the relevant standards, very valuable.

 I think we have consensus that we can ignore pre-r2.2 hardware.  What we 
actually support is `-misa-spec=<2.2|20190608|20191213>', so we can assume 
r2.2 semantics as the absolute minimum, matching the description in my 
submission.

 Also GCC documents IEEE 754-2008 semantics for the `fmin' and `fmax' RTL 
operations:

"'fminM3', 'fmaxM3'
     IEEE-conformant minimum and maximum operations.  If one operand is
     a quiet 'NaN', then the other operand is returned.  If both
     operands are quiet 'NaN', then a quiet 'NaN' is returned.  In the
     case when gcc supports signaling 'NaN' (-fsignaling-nans) an
     invalid floating point exception is raised and a quiet 'NaN' is
     returned."

and I think they need to stay as they are for the sake of IEEE 754-2008 
hardware and the `minNum' and `maxNum' (`fmin'/`fmax') operations.

 Looking further r20190608 of the RISC-V ISA specification then has[1]:

"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[2]:

"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."

 So for forwards compatibility of software built for r2.2 (such that it 
continues producing correct results with r20190608+ hardware) I think we 
need to provide `fmin'/`fmax' insns iff !HONOR_SNANS while keeping 
`smin'/`smax' insns intact.

 Then once we have IEEE 754-2019 support in place, which will require new 
RTL standard pattern names, say `fminimum'/`fmaximum', we will provide 
them iff (!HONOR_SNANS || riscv_isa_spec >= ISA_SPEC_CLASS_20190608).  It 
may be a good idea to start adding IEEE 754-2019 support, including the 
relevant `__builtin_fminimum' and `__builtin_fmaximum' intrinsics, once 
the GCC 13 development cycle has started.

 I'll post v2 then according to this consideration, once it's gone through 
regression testing.  Thanks for your input again.

References:

[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

  Maciej
  

On Sat, 22 Jan 2022, Maciej W. Rozycki wrote:

>  I think we have consensus that we can ignore pre-r2.2 hardware.  What we 
> actually support is `-misa-spec=<2.2|20190608|20191213>', so we can assume 
> r2.2 semantics as the absolute minimum, matching the description in my 
> submission.

Where, to repeat the point about possibly confusing version numbers, 
that's saying we can ignore hardware from before *ISA* revision 2.2 (which 
contained 'F' and 'D' extension version 2.0) - not that we can ignore 
hardware from before 'F' and 'D' extension version 2.2 (which changed the 
semantics of the FMIN and FMAX instructions).

>  Then once we have IEEE 754-2019 support in place, which will require new 
> RTL standard pattern names, say `fminimum'/`fmaximum', we will provide 
> them iff (!HONOR_SNANS || riscv_isa_spec >= ISA_SPEC_CLASS_20190608).  It 
> may be a good idea to start adding IEEE 754-2019 support, including the 
> relevant `__builtin_fminimum' and `__builtin_fmaximum' intrinsics, once 
> the GCC 13 development cycle has started.

The newer instruction semantics correspond to the functions fminimum_num 
and fmaximum_num, not fminimum and fmaximum (which are functions that 
treat both quiet and signaling NaNs like most libm functions do - any 
argument a NaN means the result is NaN).
  

On Mon, 24 Jan 2022, Joseph Myers wrote:

> >  I think we have consensus that we can ignore pre-r2.2 hardware.  What we 
> > actually support is `-misa-spec=<2.2|20190608|20191213>', so we can assume 
> > r2.2 semantics as the absolute minimum, matching the description in my 
> > submission.
> 
> Where, to repeat the point about possibly confusing version numbers, 
> that's saying we can ignore hardware from before *ISA* revision 2.2 (which 
> contained 'F' and 'D' extension version 2.0) - not that we can ignore 
> hardware from before 'F' and 'D' extension version 2.2 (which changed the 
> semantics of the FMIN and FMAX instructions).

 OK, I'll try to make it clear in the change description of v2.

> >  Then once we have IEEE 754-2019 support in place, which will require new 
> > RTL standard pattern names, say `fminimum'/`fmaximum', we will provide 
> > them iff (!HONOR_SNANS || riscv_isa_spec >= ISA_SPEC_CLASS_20190608).  It 
> > may be a good idea to start adding IEEE 754-2019 support, including the 
> > relevant `__builtin_fminimum' and `__builtin_fmaximum' intrinsics, once 
> > the GCC 13 development cycle has started.
> 
> The newer instruction semantics correspond to the functions fminimum_num 
> and fmaximum_num, not fminimum and fmaximum (which are functions that 
> treat both quiet and signaling NaNs like most libm functions do - any 
> argument a NaN means the result is NaN).

 I got these wrong then, sorry.  I got lost too: what is the difference 
between `fmin'/`fmax' and `fminimum'/`fmaximum' then?  It looks to me like 
we have a zoo of selection functions now with very subtle differences 
between each other.

  Maciej
  

On Wed, 26 Jan 2022, Maciej W. Rozycki wrote:

> > The newer instruction semantics correspond to the functions fminimum_num 
> > and fmaximum_num, not fminimum and fmaximum (which are functions that 
> > treat both quiet and signaling NaNs like most libm functions do - any 
> > argument a NaN means the result is NaN).
> 
>  I got these wrong then, sorry.  I got lost too: what is the difference 
> between `fmin'/`fmax' and `fminimum'/`fmaximum' then?  It looks to me like 
> we have a zoo of selection functions now with very subtle differences 
> between each other.

fmin and fmax:

* Treat quiet NaN as missing data and return the other argument (if a 
number).

* Treat signaling NaN like most functions (raise invalid, return quiet 
NaN).

fminimum and fmaximum:

* Treat quiet NaN like most functions (return quiet NaN, no exceptions 
raised unless the other argument is signaling NaN).

* Treat signaling NaN like most functions (raise invalid, return quiet 
NaN).

fminimum_num and fmaximum_num:

* Treat both quiet and signaling NaN as missing data and return the other 
argument (if a number).  "invalid" is still raised if one argument is a 
signaling NaN (contrary to the normal rule that raising "invalid" means 
also returning a quiet NaN).
  

On Wed, 26 Jan 2022, Joseph Myers wrote:

> fmin and fmax:
> 
> * Treat quiet NaN as missing data and return the other argument (if a 
> number).
> 
> * Treat signaling NaN like most functions (raise invalid, return quiet 
> NaN).
> 
> fminimum and fmaximum:
> 
> * Treat quiet NaN like most functions (return quiet NaN, no exceptions 
> raised unless the other argument is signaling NaN).
> 
> * Treat signaling NaN like most functions (raise invalid, return quiet 
> NaN).
> 
> fminimum_num and fmaximum_num:
> 
> * Treat both quiet and signaling NaN as missing data and return the other 
> argument (if a number).  "invalid" is still raised if one argument is a 
> signaling NaN (contrary to the normal rule that raising "invalid" means 
> also returning a quiet NaN).

 Thanks!

  Maciej
  

On Wed, 26 Jan 2022, Joseph Myers wrote:

> fmin and fmax:

Also:

* If the arguments are +0 and -0 in some order, it's unspecified what sign 
the result has.

> fminimum and fmaximum:

> fminimum_num and fmaximum_num:

Also:

* These four functions are required to treat -0 as less than +0.