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

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

[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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Keith Packard
In reply to this post by 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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Keith Packard
In reply to this post by 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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Kito Cheng-2
Hi Keith:

Thanks for the patch, some inline comment :)

On Tue, Jan 21, 2020 at 2:47 PM Keith Packard <[hidden email]> 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 <[hidden email]>
> ---
>  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
>
Reply | Threaded
Open this post in threaded view
|

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

Kito Cheng-2
In reply to this post by Keith Packard
Hi Keith:

LGTM, thanks :)

On Tue, Jan 21, 2020 at 2:47 PM Keith Packard <[hidden email]> wrote:

>
> Use fscsr and frcsr to store and read the FCSR register instead of
> fssr and frsr.
>
> Signed-off-by: Keith Packard <[hidden email]>
> ---
>  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
>
Reply | Threaded
Open this post in threaded view
|

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

Kito Cheng-2
In reply to this post by Keith Packard
Hi Keith:

Thanks for fixing this, LGTM.

On Tue, Jan 21, 2020 at 2:47 PM Keith Packard <[hidden email]> 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 <[hidden email]>
> ---
>  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
>
Reply | Threaded
Open this post in threaded view
|

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

Keith Packard
In reply to this post by Kito Cheng-2
Kito Cheng <[hidden email]> 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

signature.asc (847 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Keith Packard
In reply to this post by Kito Cheng-2
Kito Cheng <[hidden email]> 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

signature.asc (847 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Corinna Vinschen
In reply to this post by Keith Packard
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 <[hidden email]>
> ---
>  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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Kito Cheng-2
Hi Corinna:

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



On Tue, Jan 21, 2020 at 5:31 PM Corinna Vinschen <[hidden email]> 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 <[hidden email]>
> > ---
> >  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
Reply | Threaded
Open this post in threaded view
|

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

Keith Packard
In reply to this post by Corinna Vinschen
Corinna Vinschen <[hidden email]> 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

signature.asc (847 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Jim Wilson-2
In reply to this post by Keith Packard
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
Reply | Threaded
Open this post in threaded view
|

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

Kito Cheng-2
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 <[hidden email]> 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