On Thu, 20 Jan 2022 07:44:25 PST (-0800), ma...@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 <pal...@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")))]
