[PATCH 1/4] riscv: Use current pseudo-instructions to access the FCSR register

[PATCH 1/4] riscv: Use current pseudo-instructions to access the FCSR register

[Keith Packard]

Use fscsr and frcsr to store and read the FCSR register instead of
fssr and frsr.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 newlib/libc/machine/riscv/ieeefp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/newlib/libc/machine/riscv/ieeefp.c b/newlib/libc/machine/riscv/ieeefp.c
index 9094cc651..68ace0b09 100644
--- a/newlib/libc/machine/riscv/ieeefp.c
+++ b/newlib/libc/machine/riscv/ieeefp.c
@@ -15,14 +15,14 @@
 static void
 fssr(unsigned value)
 {
-  asm volatile ("fssr %0" :: "r"(value));
+  asm volatile ("fscsr %0" :: "r"(value));
 }
 
 static unsigned
 frsr()
 {
   unsigned value;
-  asm volatile ("frsr %0" : "=r" (value));
+  asm volatile ("frcsr %0" : "=r" (value));
   return value;
 }
 
-- 
2.25.0.rc1

[PATCH 4/4] riscv: Addfpgetroundtoi and fpsetroundtoi stubs

[Keith Packard]

I've found no description of what these functions are supposed to do,
so I'm not even going to try and implement them.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 newlib/libc/machine/riscv/ieeefp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/newlib/libc/machine/riscv/ieeefp.c b/newlib/libc/machine/riscv/ieeefp.c
index 60ecacfc2..46e4a4b15 100644
--- a/newlib/libc/machine/riscv/ieeefp.c
+++ b/newlib/libc/machine/riscv/ieeefp.c
@@ -142,3 +142,13 @@ fpsetsticky(fp_except sticky)
   return -1;
 #endif /* __riscv_flen */
 }
+
+fp_rdi fpgetroundtoi (void)
+{
+  return 0;
+}
+
+fp_rdi fpsetroundtoi (fp_rdi rdi)
+{
+  return -1;
+}
-- 
2.25.0.rc1

Re: [PATCH 4/4] riscv: Addfpgetroundtoi and fpsetroundtoi stubs

[Kito Cheng]

Hi Keith:

Thanks for the patch, some inline comment :)

On Tue, Jan 21, 2020 at 2:47 PM Keith Packard <keithp@keithp.com> wrote:
>
> I've found no description of what these functions are supposed to do,
> so I'm not even going to try and implement them.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  newlib/libc/machine/riscv/ieeefp.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/newlib/libc/machine/riscv/ieeefp.c b/newlib/libc/machine/riscv/ieeefp.c
> index 60ecacfc2..46e4a4b15 100644
> --- a/newlib/libc/machine/riscv/ieeefp.c
> +++ b/newlib/libc/machine/riscv/ieeefp.c
> @@ -142,3 +142,13 @@ fpsetsticky(fp_except sticky)
>    return -1;
>  #endif /* __riscv_flen */
>  }
> +
> +fp_rdi fpgetroundtoi (void)
> +{
> +  return 0;

According RISC-V ISA spec, V20190608 11.7:
"All floating-point to integer and integer to floating-point
conversion instructions round according to the rm field."
So seem like FP_RDI_RD would be better value if hard float is available.

#if __riscv_flen
  return FP_RDI_RD;
#else
  /* libgcc always rounding toward zero.  */
  return FP_RDI_TOZ;
#endif

> +}
> +
> +fp_rdi fpsetroundtoi (fp_rdi rdi)
> +{
> +  return -1;

According the implementation in newlib/libc/sys/sysvi386/fpx.c, the
return value seems intend to return old rounding mode for fp to
integer.

But RISC-V don't provide any way to switch that, so return -1 OK to me.

> +}
> --
> 2.25.0.rc1
>

Re: [PATCH 4/4] riscv: Addfpgetroundtoi and fpsetroundtoi stubs

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 319 bytes --]

Kito Cheng <kito.cheng@gmail.com> writes:

> According RISC-V ISA spec, V20190608 11.7:
> "All floating-point to integer and integer to floating-point
> conversion instructions round according to the rm field."
> So seem like FP_RDI_RD would be better value if hard float is
> available.

Sure.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 4/4] riscv: Addfpgetroundtoi and fpsetroundtoi stubs

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

Kito Cheng <kito.cheng@gmail.com> writes:

> According RISC-V ISA spec, V20190608 11.7:
> "All floating-point to integer and integer to floating-point
> conversion instructions round according to the rm field."
> So seem like FP_RDI_RD would be better value if hard float is available.

I just realized -- we might be able to do that on targets with hard
float and double, but many targets have only hard float and use software
for doubles. In this case, the rounding mode will differ between float
(hardware) and double (software).

So, perhaps this API should just return -1 as well...

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 4/4] riscv: Addfpgetroundtoi and fpsetroundtoi stubs

[Jim Wilson]

I think fpgetroundtoi and fpsetroundtoi should always return error.

I found a reference in a NEC V810 manual that has separate bits for FP
rounding and FP->INT conversion rounding.  Hence it needs two separate
interfaces to be able to access the two sets of rounding bits.  For
bonus points, it mentions that tkcw means task control word.  tkcw is
used in the newlib sysvi386 port implementation of these functions.
So I think some very old (pre x87?) x86 ports also had two separate
sets of rounding bits, and this is why newlib has the second set of
functions.  I didn't find a reference to confirm this though.  But for
any modern machine, there is only one set of FP rounding flags, and
hence I think the fp{get,set}roundtoi functions are useless, and
should always return error.

Jim

Re: [PATCH 4/4] riscv: Addfpgetroundtoi and fpsetroundtoi stubs

[Kito Cheng]

Hi Jim, Keith:

I agree both should return error (-1), the problem seems more
complicated if we consider long double.
But those functions are seems like useless now, so yeah, let's always
return error :P

Thanks :)

On Wed, Jan 22, 2020 at 6:43 AM Jim Wilson <jimw@sifive.com> wrote:
>
> I think fpgetroundtoi and fpsetroundtoi should always return error.
>
> I found a reference in a NEC V810 manual that has separate bits for FP
> rounding and FP->INT conversion rounding.  Hence it needs two separate
> interfaces to be able to access the two sets of rounding bits.  For
> bonus points, it mentions that tkcw means task control word.  tkcw is
> used in the newlib sysvi386 port implementation of these functions.
> So I think some very old (pre x87?) x86 ports also had two separate
> sets of rounding bits, and this is why newlib has the second set of
> functions.  I didn't find a reference to confirm this though.  But for
> any modern machine, there is only one set of FP rounding flags, and
> hence I think the fp{get,set}roundtoi functions are useless, and
> should always return error.
>
> Jim

[PATCH 3/4] riscv: Map between ieeefp.h exception bits and RISC-V FCSR bits

[Keith Packard]

If we had architecture-specific exception bits, we could just set them
to match the processor, but instead ieeefp.h is shared by all targets
so we need to map between the public values and the register contents.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 newlib/libc/machine/riscv/ieeefp.c | 40 +++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/newlib/libc/machine/riscv/ieeefp.c b/newlib/libc/machine/riscv/ieeefp.c
index c45832280..60ecacfc2 100644
--- a/newlib/libc/machine/riscv/ieeefp.c
+++ b/newlib/libc/machine/riscv/ieeefp.c
@@ -40,6 +40,40 @@ frm_fp_rnd (unsigned frm)
     }
 }
 
+static fp_except
+frm_fp_except (unsigned except)
+{
+  fp_except fp = 0;
+  if (except & (1 << 0))
+    fp |= FP_X_IMP;
+  if (except & (1 << 1))
+    fp |= FP_X_UFL;
+  if (except & (1 << 2))
+    fp |= FP_X_OFL;
+  if (except & (1 << 3))
+    fp |= FP_X_DX;
+  if (except & (1 << 4))
+    fp |= FP_X_INV;
+  return fp;
+}
+
+static unsigned
+frm_except(fp_except fp)
+{
+  unsigned except = 0;
+  if (fp & FP_X_IMP)
+    except |= (1 << 0);
+  if (fp & FP_X_UFL)
+    except |= (1 << 1);
+  if (fp & FP_X_OFL)
+    except |= (1 << 2);
+  if (fp & FP_X_DX)
+    except |= (1 << 3);
+  if (fp & FP_X_INV)
+    except |= (1 << 4);
+  return except;
+}
+
 #endif /* __riscv_flen */
 
 fp_except
@@ -63,7 +97,7 @@ fp_except
 fpgetsticky(void)
 {
 #ifdef __riscv_flen
-  return frsr () & 0x1f;
+  return frm_fp_except(frsr ());
 #else
   return 0;
 #endif /* __riscv_flen */
@@ -102,8 +136,8 @@ fpsetsticky(fp_except sticky)
 {
 #ifdef __riscv_flen
   unsigned fsr = frsr ();
-  fssr (sticky & 0x1f | fsr & ~0x1f);
-  return fsr & 0x1f;
+  fssr (frm_except(sticky) | (fsr & ~0x1f));
+  return frm_fp_except(fsr);
 #else
   return -1;
 #endif /* __riscv_flen */
-- 
2.25.0.rc1

[PATCH 2/4] riscv: Add 'break' statements to fpsetround switch

[Keith Packard]

This makes the fpsetround function actually do something rather than
just return -1 due to the default 'fall-through' behavior of the switch
statement.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 newlib/libc/machine/riscv/ieeefp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/newlib/libc/machine/riscv/ieeefp.c b/newlib/libc/machine/riscv/ieeefp.c
index 68ace0b09..c45832280 100644
--- a/newlib/libc/machine/riscv/ieeefp.c
+++ b/newlib/libc/machine/riscv/ieeefp.c
@@ -84,10 +84,10 @@ fpsetround(fp_rnd rnd_dir)
   unsigned new_rm;
   switch (rnd_dir)
     {
-    case FP_RN: new_rm = 0;
-    case FP_RZ: new_rm = 1;
-    case FP_RM: new_rm = 2;
-    case FP_RP: new_rm = 3;
+    case FP_RN: new_rm = 0; break;
+    case FP_RZ: new_rm = 1; break;
+    case FP_RM: new_rm = 2; break;
+    case FP_RP: new_rm = 3; break;
     default:    return -1;
     }
   fssr (new_rm << 5 | fsr & 0x1f);
-- 
2.25.0.rc1

Re: [PATCH 2/4] riscv: Add 'break' statements to fpsetround switch

[Kito Cheng]

Hi Keith:

Thanks for fixing this, LGTM.

On Tue, Jan 21, 2020 at 2:47 PM Keith Packard <keithp@keithp.com> wrote:
>
> This makes the fpsetround function actually do something rather than
> just return -1 due to the default 'fall-through' behavior of the switch
> statement.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  newlib/libc/machine/riscv/ieeefp.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/newlib/libc/machine/riscv/ieeefp.c b/newlib/libc/machine/riscv/ieeefp.c
> index 68ace0b09..c45832280 100644
> --- a/newlib/libc/machine/riscv/ieeefp.c
> +++ b/newlib/libc/machine/riscv/ieeefp.c
> @@ -84,10 +84,10 @@ fpsetround(fp_rnd rnd_dir)
>    unsigned new_rm;
>    switch (rnd_dir)
>      {
> -    case FP_RN: new_rm = 0;
> -    case FP_RZ: new_rm = 1;
> -    case FP_RM: new_rm = 2;
> -    case FP_RP: new_rm = 3;
> +    case FP_RN: new_rm = 0; break;
> +    case FP_RZ: new_rm = 1; break;
> +    case FP_RM: new_rm = 2; break;
> +    case FP_RP: new_rm = 3; break;
>      default:    return -1;
>      }
>    fssr (new_rm << 5 | fsr & 0x1f);
> --
> 2.25.0.rc1
>

Re: [PATCH 1/4] riscv: Use current pseudo-instructions to access the FCSR register

[Kito Cheng]

Hi Keith:

LGTM, thanks :)

On Tue, Jan 21, 2020 at 2:47 PM Keith Packard <keithp@keithp.com> wrote:
>
> Use fscsr and frcsr to store and read the FCSR register instead of
> fssr and frsr.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  newlib/libc/machine/riscv/ieeefp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/newlib/libc/machine/riscv/ieeefp.c b/newlib/libc/machine/riscv/ieeefp.c
> index 9094cc651..68ace0b09 100644
> --- a/newlib/libc/machine/riscv/ieeefp.c
> +++ b/newlib/libc/machine/riscv/ieeefp.c
> @@ -15,14 +15,14 @@
>  static void
>  fssr(unsigned value)
>  {
> -  asm volatile ("fssr %0" :: "r"(value));
> +  asm volatile ("fscsr %0" :: "r"(value));
>  }
>
>  static unsigned
>  frsr()
>  {
>    unsigned value;
> -  asm volatile ("frsr %0" : "=r" (value));
> +  asm volatile ("frcsr %0" : "=r" (value));
>    return value;
>  }
>
> --
> 2.25.0.rc1
>

Re: [PATCH 1/4] riscv: Use current pseudo-instructions to access the FCSR register

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

On Jan 20 22:46, Keith Packard wrote:
> Use fscsr and frcsr to store and read the FCSR register instead of
> fssr and frsr.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  newlib/libc/machine/riscv/ieeefp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/newlib/libc/machine/riscv/ieeefp.c b/newlib/libc/machine/riscv/ieeefp.c
> index 9094cc651..68ace0b09 100644
> --- a/newlib/libc/machine/riscv/ieeefp.c
> +++ b/newlib/libc/machine/riscv/ieeefp.c
> @@ -15,14 +15,14 @@
>  static void
>  fssr(unsigned value)
>  {
> -  asm volatile ("fssr %0" :: "r"(value));
> +  asm volatile ("fscsr %0" :: "r"(value));
>  }
>  
>  static unsigned
>  frsr()
>  {
>    unsigned value;
> -  asm volatile ("frsr %0" : "=r" (value));
> +  asm volatile ("frcsr %0" : "=r" (value));
>    return value;
>  }
>  
> -- 
> 2.25.0.rc1

Patches 1-3 pushed.  Patch 4 is still under discussion, iiuc?

Keith, a --cover-letter for patch series would be nice, if only
to allow patchset-wide discussions to appear in a neutral place :)


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] riscv: Use current pseudo-instructions to access the FCSR register

[Kito Cheng]

Hi Corinna:

Yeah, [4/4]  is still under discussion, thanks :)



On Tue, Jan 21, 2020 at 5:31 PM Corinna Vinschen <vinschen@redhat.com> wrote:
>
> On Jan 20 22:46, Keith Packard wrote:
> > Use fscsr and frcsr to store and read the FCSR register instead of
> > fssr and frsr.
> >
> > Signed-off-by: Keith Packard <keithp@keithp.com>
> > ---
> >  newlib/libc/machine/riscv/ieeefp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/newlib/libc/machine/riscv/ieeefp.c b/newlib/libc/machine/riscv/ieeefp.c
> > index 9094cc651..68ace0b09 100644
> > --- a/newlib/libc/machine/riscv/ieeefp.c
> > +++ b/newlib/libc/machine/riscv/ieeefp.c
> > @@ -15,14 +15,14 @@
> >  static void
> >  fssr(unsigned value)
> >  {
> > -  asm volatile ("fssr %0" :: "r"(value));
> > +  asm volatile ("fscsr %0" :: "r"(value));
> >  }
> >
> >  static unsigned
> >  frsr()
> >  {
> >    unsigned value;
> > -  asm volatile ("frsr %0" : "=r" (value));
> > +  asm volatile ("frcsr %0" : "=r" (value));
> >    return value;
> >  }
> >
> > --
> > 2.25.0.rc1
>
> Patches 1-3 pushed.  Patch 4 is still under discussion, iiuc?
>
> Keith, a --cover-letter for patch series would be nice, if only
> to allow patchset-wide discussions to appear in a neutral place :)
>
>
> Thanks,
> Corinna
>
> --
> Corinna Vinschen
> Cygwin Maintainer
> Red Hat

Re: [PATCH 1/4] riscv: Use current pseudo-instructions to access the FCSR register

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 330 bytes --]

Corinna Vinschen <vinschen@redhat.com> writes:

> Keith, a --cover-letter for patch series would be nice, if only
> to allow patchset-wide discussions to appear in a neutral place :)

Oops! So sorry I didn't send one along. I'll add that to my patch
submission process notes so I don't forget in the future.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]