On Thu, Jan 20, 2022 at 12:30 PM Palmer Dabbelt <pal...@dabbelt.com> wrote: > > 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.
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 <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")))]
