[v2] RISC-V: Fix one bug for floating-point static frm

  From: Pan Li <pan2.li@intel.com>

This patch would like to fix one bug to align below items of spec.

1. By default, the RVV floating-point will take dyn mode.
2. DYN is invalid in FRM register for RVV floating-point.

When mode switching the function entry and exit, it will take DYN as
the frm mode.

Signed-off-by: Pan Li <pan2.li@intel.com>

gcc/ChangeLog:

	* config/riscv/riscv.cc (riscv_emit_mode_set): Avoid emit insn
	when FRM_MODE_DYN.
	(riscv_mode_entry): Take FRM_MODE_DYN as entry mode.
	(riscv_mode_exit): Likewise for exit mode.
	(riscv_mode_needed): Likewise for needed mode.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/rvv/base/float-point-frm-insert-6.c: New test.
---
 gcc/config/riscv/riscv.cc                     |  8 ++---
 .../riscv/rvv/base/float-point-frm-insert-6.c | 31 +++++++++++++++++++
 2 files changed, 35 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/float-point-frm-insert-6.c
  

LGTM.



juzhe.zhong@rivai.ai
 
From: pan2.li
Date: 2023-07-04 16:08
To: gcc-patches
CC: juzhe.zhong; rdapp.gcc; jeffreyalaw; pan2.li; yanzhang.wang; kito.cheng
Subject: [PATCH v2] RISC-V: Fix one bug for floating-point static frm
From: Pan Li <pan2.li@intel.com>
 
This patch would like to fix one bug to align below items of spec.
 
1. By default, the RVV floating-point will take dyn mode.
2. DYN is invalid in FRM register for RVV floating-point.
 
When mode switching the function entry and exit, it will take DYN as
the frm mode.
 
Signed-off-by: Pan Li <pan2.li@intel.com>
 
gcc/ChangeLog:
 
* config/riscv/riscv.cc (riscv_emit_mode_set): Avoid emit insn
when FRM_MODE_DYN.
(riscv_mode_entry): Take FRM_MODE_DYN as entry mode.
(riscv_mode_exit): Likewise for exit mode.
(riscv_mode_needed): Likewise for needed mode.
 
gcc/testsuite/ChangeLog:
 
* gcc.target/riscv/rvv/base/float-point-frm-insert-6.c: New test.
---
gcc/config/riscv/riscv.cc                     |  8 ++---
.../riscv/rvv/base/float-point-frm-insert-6.c | 31 +++++++++++++++++++
2 files changed, 35 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/float-point-frm-insert-6.c
 
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index e4dc8115e69..37f96f8a238 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -7670,7 +7670,7 @@ riscv_emit_mode_set (int entity, int mode, int prev_mode,
emit_insn (gen_vxrmsi (gen_int_mode (mode, SImode)));
       break;
     case RISCV_FRM:
-      if (mode != FRM_MODE_NONE && mode != prev_mode)
+      if (mode != FRM_MODE_DYN && mode != prev_mode)
{
  rtx scaler = gen_reg_rtx (SImode);
  rtx imm = gen_int_mode (mode, SImode);
@@ -7697,7 +7697,7 @@ riscv_mode_needed (int entity, rtx_insn *insn)
     case RISCV_VXRM:
       return code >= 0 ? get_attr_vxrm_mode (insn) : VXRM_MODE_NONE;
     case RISCV_FRM:
-      return code >= 0 ? get_attr_frm_mode (insn) : FRM_MODE_NONE;
+      return code >= 0 ? get_attr_frm_mode (insn) : FRM_MODE_DYN;
     default:
       gcc_unreachable ();
     }
@@ -7774,7 +7774,7 @@ riscv_mode_entry (int entity)
     case RISCV_VXRM:
       return VXRM_MODE_NONE;
     case RISCV_FRM:
-      return FRM_MODE_NONE;
+      return FRM_MODE_DYN;
     default:
       gcc_unreachable ();
     }
@@ -7791,7 +7791,7 @@ riscv_mode_exit (int entity)
     case RISCV_VXRM:
       return VXRM_MODE_NONE;
     case RISCV_FRM:
-      return FRM_MODE_NONE;
+      return FRM_MODE_DYN;
     default:
       gcc_unreachable ();
     }
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/float-point-frm-insert-6.c b/gcc/testsuite/gcc.target/riscv/rvv/base/float-point-frm-insert-6.c
new file mode 100644
index 00000000000..6d896e0953e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rvv/base/float-point-frm-insert-6.c
@@ -0,0 +1,31 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gcv -mabi=lp64 -O3 -Wno-psabi" } */
+
+#include "riscv_vector.h"
+
+typedef float float32_t;
+
+vfloat32m1_t
+test_riscv_vfadd_vv_f32m1_rm (vfloat32m1_t op1, vfloat32m1_t op2, size_t vl) {
+  return __riscv_vfadd_vv_f32m1_rm (op1, op2, 7, vl);
+}
+
+vfloat32m1_t
+test_vfadd_vv_f32m1_m_rm(vbool32_t mask, vfloat32m1_t op1, vfloat32m1_t op2,
+ size_t vl) {
+  return __riscv_vfadd_vv_f32m1_m_rm(mask, op1, op2, 7, vl);
+}
+
+vfloat32m1_t
+test_vfadd_vf_f32m1_rm(vfloat32m1_t op1, float32_t op2, size_t vl) {
+  return __riscv_vfadd_vf_f32m1_rm(op1, op2, 7, vl);
+}
+
+vfloat32m1_t
+test_vfadd_vf_f32m1_m_rm(vbool32_t mask, vfloat32m1_t op1, float32_t op2,
+ size_t vl) {
+  return __riscv_vfadd_vf_f32m1_m_rm(mask, op1, op2, 7, vl);
+}
+
+/* { dg-final { scan-assembler-times {vfadd\.v[vf]\s+v[0-9]+,\s*v[0-9]+,\s*[fav]+[0-9]+} 4 } } */
+/* { dg-final { scan-assembler-not {fsrm\s+[ax][0-9]+,\s*[ax][0-9]+} } } */
-- 
2.34.1
  

Hi Pan,

I only just now got back to my mails and I'm a bit confused about
the several patches related to rounding mode.

> 1. By default, the RVV floating-point will take dyn mode.

Here you are referring to 10.1 in the spec I assume.  Could we
add this as a comment in the code?

> 2. DYN is invalid in FRM register for RVV floating-point.

What does that mean or rather how is that reflected in the code?

> -      return code >= 0 ? get_attr_frm_mode (insn) : FRM_MODE_NONE;
> +      return code >= 0 ? get_attr_frm_mode (insn) : FRM_MODE_DYN;
This still has yesterday's bug right?  I.e. get_attr_frm_mode returns
5 for dyn instead of 7 because the enums don't match (leading to
SIGILL).

> +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/float-point-frm-insert-6.c
> @@ -0,0 +1,31 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gcv -mabi=lp64 -O3 -Wno-psabi" } */

I would prefer to have an execution test here as well.  Even
though we likely FAIL in other tests when the rounding mode
is off, it would be good to have a specific one.  Maybe it
doesn't exactly fit into this patch but in general.

Regards
 Robin
  

Hi Robin,

Thanks for reviewing, sorry for my silly mistakes in the original patch, CIL.

> Here you are referring to 10.1 in the spec I assume.  Could we add this as a comment in the code?

Sure, from the spec rvv 1.0, aka "All standard vector floating-point arithmetic operations follow the IEEE-754/2008 standard. All vector floating-point operations use the dynamic rounding mode in the frm register".

> What does that mean or rather how is that reflected in the code?

This part from spec 20191213 F part, aka "111 DYN In instruction’s rm fi eld, selects dynamic rounding mode; In Rounding Mode register, Invalid.".

> This still has yesterday's bug right?  I.e. get_attr_frm_mode returns 5 for dyn instead of 7 because the enums don't match (leading to SIGILL).

Should be no, FRM_MODE_DYN is generated by vector.md, which present the mode for switching and different from the frm value defined in riscv-v.cc. However and more generally, we should have a function convert from frm_mode to frm as I understand.

> I would prefer to have an execution test here as well.  Even
> though we likely FAIL in other tests when the rounding mode
> is off, it would be good to have a specific one.  Maybe it
> doesn't exactly fit into this patch but in general.

Sure, I can file another PATCH for execution, like vfadd with RMM mode but the frm value is changed by this intrinsic, the underlying dynamic round mode can leverage this case I bet.

Pan

-----Original Message-----
From: Robin Dapp <rdapp.gcc@gmail.com> 
Sent: Tuesday, July 4, 2023 8:52 PM
To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
Cc: rdapp.gcc@gmail.com; juzhe.zhong@rivai.ai; jeffreyalaw@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com
Subject: Re: [PATCH v2] RISC-V: Fix one bug for floating-point static frm

Hi Pan,

I only just now got back to my mails and I'm a bit confused about
the several patches related to rounding mode.

> 1. By default, the RVV floating-point will take dyn mode.

Here you are referring to 10.1 in the spec I assume.  Could we
add this as a comment in the code?

> 2. DYN is invalid in FRM register for RVV floating-point.

What does that mean or rather how is that reflected in the code?

> -      return code >= 0 ? get_attr_frm_mode (insn) : FRM_MODE_NONE;
> +      return code >= 0 ? get_attr_frm_mode (insn) : FRM_MODE_DYN;
This still has yesterday's bug right?  I.e. get_attr_frm_mode returns
5 for dyn instead of 7 because the enums don't match (leading to
SIGILL).

> +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/float-point-frm-insert-6.c
> @@ -0,0 +1,31 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gcv -mabi=lp64 -O3 -Wno-psabi" } */

I would prefer to have an execution test here as well.  Even
though we likely FAIL in other tests when the rounding mode
is off, it would be good to have a specific one.  Maybe it
doesn't exactly fit into this patch but in general.

Regards
 Robin