diff mbox series

[v2,10/18] RISC-V: Hard float support for 32-bit

Message ID ba0fe98b9606d68e367c307a7ccfe617c541ed73.1591201405.git.alistair.francis@wdc.com
State Committed
Headers show
Series glibc port for 32-bit RISC-V (RV32) | expand

Commit Message

Alistair Francis June 3, 2020, 4:25 p.m. UTC
From: Zong Li <zongbox@gmail.com>

This patch contains hardware floating-point support for the RV32IF and
RV32IFD
---
 sysdeps/riscv/rv32/rvd/s_lrint.c   | 31 ++++++++++++++++++++++++++++++
 sysdeps/riscv/rv32/rvd/s_lround.c  | 31 ++++++++++++++++++++++++++++++
 sysdeps/riscv/rv32/rvf/s_lrintf.c  | 31 ++++++++++++++++++++++++++++++
 sysdeps/riscv/rv32/rvf/s_lroundf.c | 31 ++++++++++++++++++++++++++++++
 4 files changed, 124 insertions(+)
 create mode 100644 sysdeps/riscv/rv32/rvd/s_lrint.c
 create mode 100644 sysdeps/riscv/rv32/rvd/s_lround.c
 create mode 100644 sysdeps/riscv/rv32/rvf/s_lrintf.c
 create mode 100644 sysdeps/riscv/rv32/rvf/s_lroundf.c

Comments

Maciej W. Rozycki July 11, 2020, 12:49 a.m. UTC | #1
On Wed, 3 Jun 2020, Alistair Francis via Libc-alpha wrote:

> This patch contains hardware floating-point support for the RV32IF and
> RV32IFD

 Full stop please.

> diff --git a/sysdeps/riscv/rv32/rvd/s_lrint.c b/sysdeps/riscv/rv32/rvd/s_lrint.c
> new file mode 100644
> index 0000000000..df406aacb6
> --- /dev/null
> +++ b/sysdeps/riscv/rv32/rvd/s_lrint.c
> @@ -0,0 +1,31 @@
> +/* lrint().  RISC-V version.

 I think this has to mention this is the 32-bit version somehow, like 
"32-bit RISC-V" or suchlike ("RV32" might be too cryptic/slang).  Feel 
free to find a better wording (I'm not particularly happy to start a 
sentence with a number).

> +   Copyright (C) 2017-2020 Free Software Foundation, Inc.

 Again, 2020 only, and likewise throughout.  Also I missed one case in 
01/18 and may have elsewhere, please double-check the remaining patches.

 Otherwise OK as far as code already proposed for this change is 
concerned.

 What about the other math functions though?  We have a lot of optimised 
versions in `sysdeps/riscv/rv64/rv{f,d}/', which seem suitable for RV32 as 
it stands, such as `s_llrintf.c' or `s_nearbyint.c'.  Instead we build 
generic `sysdeps/ieee754/{flt-32,dbl-64}/' variants.

 Shouldn't we also move the XLEN-agnostic optimised functions to 
`sysdeps/riscv/rv{f,d}/' with this change?  I think we only need to keep 
those that use the `long int' type at the interface in the XLEN-specific 
directory.

  Maciej
Alistair Francis July 11, 2020, 3:49 p.m. UTC | #2
On Fri, Jul 10, 2020 at 5:49 PM Maciej W. Rozycki via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On Wed, 3 Jun 2020, Alistair Francis via Libc-alpha wrote:
>
> > This patch contains hardware floating-point support for the RV32IF and
> > RV32IFD
>
>  Full stop please.

Fixed.

>
> > diff --git a/sysdeps/riscv/rv32/rvd/s_lrint.c b/sysdeps/riscv/rv32/rvd/s_lrint.c
> > new file mode 100644
> > index 0000000000..df406aacb6
> > --- /dev/null
> > +++ b/sysdeps/riscv/rv32/rvd/s_lrint.c
> > @@ -0,0 +1,31 @@
> > +/* lrint().  RISC-V version.
>
>  I think this has to mention this is the 32-bit version somehow, like
> "32-bit RISC-V" or suchlike ("RV32" might be too cryptic/slang).  Feel
> free to find a better wording (I'm not particularly happy to start a
> sentence with a number).

Fixed.

>
> > +   Copyright (C) 2017-2020 Free Software Foundation, Inc.
>
>  Again, 2020 only, and likewise throughout.  Also I missed one case in
> 01/18 and may have elsewhere, please double-check the remaining patches.

Fixed in the whole series.

>
>  Otherwise OK as far as code already proposed for this change is
> concerned.

Thanks

>
>  What about the other math functions though?  We have a lot of optimised
> versions in `sysdeps/riscv/rv64/rv{f,d}/', which seem suitable for RV32 as
> it stands, such as `s_llrintf.c' or `s_nearbyint.c'.  Instead we build
> generic `sysdeps/ieee754/{flt-32,dbl-64}/' variants.

I haven't looked into the others, but I think you are right and this
can be improved.

>
>  Shouldn't we also move the XLEN-agnostic optimised functions to
> `sysdeps/riscv/rv{f,d}/' with this change?  I think we only need to keep
> those that use the `long int' type at the interface in the XLEN-specific
> directory.

They are all xlen dependent though. All of the 64-bit ones use
fcvt.l.d (or something similar) which doesn't exist on RV32.

Although the functions end up being the same, the actual assembly
instruction called is different. We can look at consolidating them
into a single file and do a xlen/__WORDSIZE macro check.

At this stage in glibc development I don't really want to change the
floating point helpers. Can we leave this as is and then for the next
release I can consolidate all of these into single files that do
xlen/__WORDSIZE #ifdefs? That way we will just have a single file (for
each operation) for RISC-V that will call a different assembly
instruction based on xlen or __WORDSIZE.

Alistair

>
>   Maciej
Maciej W. Rozycki July 11, 2020, 10:13 p.m. UTC | #3
On Sat, 11 Jul 2020, Alistair Francis wrote:

> >  Shouldn't we also move the XLEN-agnostic optimised functions to
> > `sysdeps/riscv/rv{f,d}/' with this change?  I think we only need to keep
> > those that use the `long int' type at the interface in the XLEN-specific
> > directory.
> 
> They are all xlen dependent though. All of the 64-bit ones use
> fcvt.l.d (or something similar) which doesn't exist on RV32.

 Umm, right, I missed this detail, sorry for the confusion.

> Although the functions end up being the same, the actual assembly
> instruction called is different. We can look at consolidating them
> into a single file and do a xlen/__WORDSIZE macro check.

 I don't think there is a simple alternative available for RV32 that would 
be worth keeping together with the RV64 variant.  Did I miss anything?  
What instruction(s) do you have in mind?

> At this stage in glibc development I don't really want to change the
> floating point helpers. Can we leave this as is and then for the next
> release I can consolidate all of these into single files that do
> xlen/__WORDSIZE #ifdefs? That way we will just have a single file (for
> each operation) for RISC-V that will call a different assembly
> instruction based on xlen or __WORDSIZE.

 Sure, in these circumstances this change is fine with me then.

  Maciej
Alistair Francis July 12, 2020, 3:34 p.m. UTC | #4
On Sat, Jul 11, 2020 at 3:14 PM Maciej W. Rozycki <macro@wdc.com> wrote:
>
> On Sat, 11 Jul 2020, Alistair Francis wrote:
>
> > >  Shouldn't we also move the XLEN-agnostic optimised functions to
> > > `sysdeps/riscv/rv{f,d}/' with this change?  I think we only need to keep
> > > those that use the `long int' type at the interface in the XLEN-specific
> > > directory.
> >
> > They are all xlen dependent though. All of the 64-bit ones use
> > fcvt.l.d (or something similar) which doesn't exist on RV32.
>
>  Umm, right, I missed this detail, sorry for the confusion.
>
> > Although the functions end up being the same, the actual assembly
> > instruction called is different. We can look at consolidating them
> > into a single file and do a xlen/__WORDSIZE macro check.
>
>  I don't think there is a simple alternative available for RV32 that would
> be worth keeping together with the RV64 variant.  Did I miss anything?
> What instruction(s) do you have in mind?

Something like this for the generic RISC-V lround:

long int
__lroundf (float x)
{
#if __WORDSIZE == 64
  int64_t res;
  asm ("fcvt.l.s %0, %1, rmm" : "=r" (res) : "f" (x));
#else
  int32_t res;
  asm ("fcvt.w.s %0, %1, rmm" : "=r" (res) : "f" (x));
#endif
  return res;
}

I'm not sure if it's clearer, but for some of the more complex
functions (roundeven for example) it might be easier.

It also means if there is a bug fixed in one it'll end up fixed for both.

>
> > At this stage in glibc development I don't really want to change the
> > floating point helpers. Can we leave this as is and then for the next
> > release I can consolidate all of these into single files that do
> > xlen/__WORDSIZE #ifdefs? That way we will just have a single file (for
> > each operation) for RISC-V that will call a different assembly
> > instruction based on xlen or __WORDSIZE.
>
>  Sure, in these circumstances this change is fine with me then.

Great! Thanks

Alistair

>
>   Maciej
Maciej W. Rozycki July 12, 2020, 10:10 p.m. UTC | #5
On Sun, 12 Jul 2020, Alistair Francis wrote:

> >  I don't think there is a simple alternative available for RV32 that would
> > be worth keeping together with the RV64 variant.  Did I miss anything?
> > What instruction(s) do you have in mind?
> 
> Something like this for the generic RISC-V lround:
> 
> long int
> __lroundf (float x)
> {
> #if __WORDSIZE == 64
>   int64_t res;
>   asm ("fcvt.l.s %0, %1, rmm" : "=r" (res) : "f" (x));
> #else
>   int32_t res;
>   asm ("fcvt.w.s %0, %1, rmm" : "=r" (res) : "f" (x));
> #endif
>   return res;
> }
> 
> I'm not sure if it's clearer, but for some of the more complex
> functions (roundeven for example) it might be easier.
> 
> It also means if there is a bug fixed in one it'll end up fixed for both.

 Ah, you mean the `float' to `long int' conversion functions, necessarily 
ABI-specific due to the changing width of the latter data type.  Well, I 
meant the operations involving FCVT.L.D/FCVT.D.L.  I can see no RISC-V 
solution for them that would surpass the generic implementation.

 As far as your example above is concerned if we decided to merge the 
files at all, I would reduce it to:

#if __WORDSIZE == 64
# define OP "fcvt.l.s"
#elif __WORDSIZE == 32
# define OP "fcvt.w.s"
#else
# error Unsupported
#endif

long int
__lroundf (float x)
{
  long int res;
  asm (OP "\t%0, %1, rmm" : "=r" (res) : "f" (x));
  return res;
}

or suchlike (I'm not sure if there's any gain here from `res' having an 
explicit-width data type).  Likewise with the rest.

  Maciej
Alistair Francis Aug. 27, 2020, 6:36 p.m. UTC | #6
On Sun, Jul 12, 2020 at 3:10 PM Maciej W. Rozycki <macro@wdc.com> wrote:
>
> On Sun, 12 Jul 2020, Alistair Francis wrote:
>
> > >  I don't think there is a simple alternative available for RV32 that would
> > > be worth keeping together with the RV64 variant.  Did I miss anything?
> > > What instruction(s) do you have in mind?
> >
> > Something like this for the generic RISC-V lround:
> >
> > long int
> > __lroundf (float x)
> > {
> > #if __WORDSIZE == 64
> >   int64_t res;
> >   asm ("fcvt.l.s %0, %1, rmm" : "=r" (res) : "f" (x));
> > #else
> >   int32_t res;
> >   asm ("fcvt.w.s %0, %1, rmm" : "=r" (res) : "f" (x));
> > #endif
> >   return res;
> > }
> >
> > I'm not sure if it's clearer, but for some of the more complex
> > functions (roundeven for example) it might be easier.
> >
> > It also means if there is a bug fixed in one it'll end up fixed for both.
>
>  Ah, you mean the `float' to `long int' conversion functions, necessarily
> ABI-specific due to the changing width of the latter data type.  Well, I
> meant the operations involving FCVT.L.D/FCVT.D.L.  I can see no RISC-V
> solution for them that would surpass the generic implementation.
>
>  As far as your example above is concerned if we decided to merge the
> files at all, I would reduce it to:
>
> #if __WORDSIZE == 64
> # define OP "fcvt.l.s"
> #elif __WORDSIZE == 32
> # define OP "fcvt.w.s"
> #else
> # error Unsupported
> #endif
>
> long int
> __lroundf (float x)
> {
>   long int res;
>   asm (OP "\t%0, %1, rmm" : "=r" (res) : "f" (x));
>   return res;
> }
>
> or suchlike (I'm not sure if there's any gain here from `res' having an
> explicit-width data type).  Likewise with the rest.

Sounds good. Now that RV32 support is in I have some patches for this,
I'll send them out soon.

Alistair

>
>   Maciej
diff mbox series

Patch

diff --git a/sysdeps/riscv/rv32/rvd/s_lrint.c b/sysdeps/riscv/rv32/rvd/s_lrint.c
new file mode 100644
index 0000000000..df406aacb6
--- /dev/null
+++ b/sysdeps/riscv/rv32/rvd/s_lrint.c
@@ -0,0 +1,31 @@ 
+/* lrint().  RISC-V version.
+   Copyright (C) 2017-2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library.  If not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <math.h>
+#include <libm-alias-double.h>
+#include <stdint.h>
+
+long int
+__lrint (double x)
+{
+  int32_t res;
+  asm ("fcvt.w.d %0, %1" : "=r" (res) : "f" (x));
+  return res;
+}
+
+libm_alias_double (__lrint, lrint)
diff --git a/sysdeps/riscv/rv32/rvd/s_lround.c b/sysdeps/riscv/rv32/rvd/s_lround.c
new file mode 100644
index 0000000000..72aa2b179d
--- /dev/null
+++ b/sysdeps/riscv/rv32/rvd/s_lround.c
@@ -0,0 +1,31 @@ 
+/* lround().  RISC-V version.
+   Copyright (C) 2017-2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library.  If not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <math.h>
+#include <libm-alias-double.h>
+#include <stdint.h>
+
+long int
+__lround (double x)
+{
+  int32_t res;
+  asm ("fcvt.w.d %0, %1, rmm" : "=r" (res) : "f" (x));
+  return res;
+}
+
+libm_alias_double (__lround, lround)
diff --git a/sysdeps/riscv/rv32/rvf/s_lrintf.c b/sysdeps/riscv/rv32/rvf/s_lrintf.c
new file mode 100644
index 0000000000..08d44fa738
--- /dev/null
+++ b/sysdeps/riscv/rv32/rvf/s_lrintf.c
@@ -0,0 +1,31 @@ 
+/* lrintf().  RISC-V version.
+   Copyright (C) 2017-2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library.  If not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <math.h>
+#include <libm-alias-float.h>
+#include <stdint.h>
+
+long int
+__lrintf (float x)
+{
+  int32_t res;
+  asm ("fcvt.w.s %0, %1" : "=r" (res) : "f" (x));
+  return res;
+}
+
+libm_alias_float (__lrint, lrint)
diff --git a/sysdeps/riscv/rv32/rvf/s_lroundf.c b/sysdeps/riscv/rv32/rvf/s_lroundf.c
new file mode 100644
index 0000000000..9ff5d63d1c
--- /dev/null
+++ b/sysdeps/riscv/rv32/rvf/s_lroundf.c
@@ -0,0 +1,31 @@ 
+/* lroundf().  RISC-V version.
+   Copyright (C) 2017-2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library.  If not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <math.h>
+#include <libm-alias-float.h>
+#include <stdint.h>
+
+long int
+__lroundf (float x)
+{
+  int32_t res;
+  asm ("fcvt.w.s %0, %1, rmm" : "=r" (res) : "f" (x));
+  return res;
+}
+
+libm_alias_float (__lround, lround)