RE: [PATCH v4] RISC-V: Fix one bug for floating-point static frm
Thanks Robin for reviewing, will address the comments with PATCH v5 later as I
am in the middle of sth.
> In riscv_mode_after the default mode is again FRM_MODE_NONE. Wouldn't
> we also want FRM_MODE_DYN here?
All of FRM should be aligned to DYN in PATCH v4, will double check about it
when prepare the v5.
Pan
-Original Message-
From: Robin Dapp
Sent: Wednesday, July 5, 2023 4:03 PM
To: Li, Pan2 ; gcc-patches@gcc.gnu.org
Cc: rdapp@gmail.com; juzhe.zh...@rivai.ai; jeffreya...@gmail.com; Wang,
Yanzhang ; kito.ch...@gmail.com
Subject: Re: [PATCH v4] RISC-V: Fix one bug for floating-point static frm
Hi Pan,
yes, the problem is fixed for me. Still some comments ;) Sorry
it took a while.
> 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.
We need to clarify this as it is misleading (even if it's just
a patch description, at least I was confused):
RVV floating-point instructions always (implicitly) use the dynamic
rounding mode. That's IMHO not a default but rather an unchangeable
fact. This implies that rounding is performed according to the
rounding mode set in the FRM register. The FRM register itself
only holds proper rounding modes and never the dynamic rounding mode.
> - if (mode != FRM_MODE_NONE && mode != prev_mode)
> + if (mode != FRM_MODE_DYN && mode != prev_mode)
> {
Adding a comment like "Switching to the dynamic rounding mode is not
necessary. When an instruction requests it, it effectively uses
the rounding mode already set in the FRM register. All other rounding
modes require us to switch the rounding mode via the FRM register."
> - return code >= 0 ? get_attr_frm_mode (insn) : FRM_MODE_NONE;
> + /* According to RVV 1.0 spec, all vector floating-point operations use
> + the dynamic rounding mode in the frm register. */
> + return code >= 0 ? get_attr_frm_mode (insn) : FRM_MODE_DYN;
As you reverted the previous patch get_attr_frm_mode is no longer
problematic because it returns FRM_MODE_NONE for instructions with
a dynamic rounding mode (instead of FRM_MODE_DYN). I still find
that a bit confusing or at least halfway inconsistent and somebody
reading it will suppose something is wrong. Could you either fix
the enum or add a TODO here that explains the situation?
The normal flow is that mode switching asks us if we need a mode
switch for an instruction and returning "NO MODE" means no. But
we return FRM_MODE_DYN by default and FRM_MODE_NONE for vector float
which appears odd.
In riscv_mode_after the default mode is again FRM_MODE_NONE. Wouldn't
we also want FRM_MODE_DYN here?
> @@ -7791,7 +7795,9 @@ riscv_mode_exit (int entity)
> case RISCV_VXRM:
>return VXRM_MODE_NONE;
> case RISCV_FRM:
> - return FRM_MODE_NONE;
> + /* According to RVV 1.0 spec, all vector floating-point operations use
> + the dynamic rounding mode in the frm register. */
> + return FRM_MODE_DYN;
I'd rather not have the comment duplicated all over the place. I
know I asked for it but I'd rather have it at a single spot explaining
what we need to do.
Regards
Robin
Re: [PATCH v4] RISC-V: Fix one bug for floating-point static frm
Hi Pan,
yes, the problem is fixed for me. Still some comments ;) Sorry
it took a while.
> 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.
We need to clarify this as it is misleading (even if it's just
a patch description, at least I was confused):
RVV floating-point instructions always (implicitly) use the dynamic
rounding mode. That's IMHO not a default but rather an unchangeable
fact. This implies that rounding is performed according to the
rounding mode set in the FRM register. The FRM register itself
only holds proper rounding modes and never the dynamic rounding mode.
> - if (mode != FRM_MODE_NONE && mode != prev_mode)
> + if (mode != FRM_MODE_DYN && mode != prev_mode)
> {
Adding a comment like "Switching to the dynamic rounding mode is not
necessary. When an instruction requests it, it effectively uses
the rounding mode already set in the FRM register. All other rounding
modes require us to switch the rounding mode via the FRM register."
> - return code >= 0 ? get_attr_frm_mode (insn) : FRM_MODE_NONE;
> + /* According to RVV 1.0 spec, all vector floating-point operations use
> + the dynamic rounding mode in the frm register. */
> + return code >= 0 ? get_attr_frm_mode (insn) : FRM_MODE_DYN;
As you reverted the previous patch get_attr_frm_mode is no longer
problematic because it returns FRM_MODE_NONE for instructions with
a dynamic rounding mode (instead of FRM_MODE_DYN). I still find
that a bit confusing or at least halfway inconsistent and somebody
reading it will suppose something is wrong. Could you either fix
the enum or add a TODO here that explains the situation?
The normal flow is that mode switching asks us if we need a mode
switch for an instruction and returning "NO MODE" means no. But
we return FRM_MODE_DYN by default and FRM_MODE_NONE for vector float
which appears odd.
In riscv_mode_after the default mode is again FRM_MODE_NONE. Wouldn't
we also want FRM_MODE_DYN here?
> @@ -7791,7 +7795,9 @@ riscv_mode_exit (int entity)
> case RISCV_VXRM:
>return VXRM_MODE_NONE;
> case RISCV_FRM:
> - return FRM_MODE_NONE;
> + /* According to RVV 1.0 spec, all vector floating-point operations use
> + the dynamic rounding mode in the frm register. */
> + return FRM_MODE_DYN;
I'd rather not have the comment duplicated all over the place. I
know I asked for it but I'd rather have it at a single spot explaining
what we need to do.
Regards
Robin
RE: [PATCH v4] RISC-V: Fix one bug for floating-point static frm
Thanks Robin, it passed all tests of riscv.exp and rvv.exp from my side. Could you please help to double confirm the issue you meet is resolved or not? Pan -Original Message- From: Robin Dapp Sent: Wednesday, July 5, 2023 3:11 PM To: Kito Cheng ; Li, Pan2 Cc: rdapp@gmail.com; gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; jeffreya...@gmail.com; Wang, Yanzhang Subject: Re: [PATCH v4] RISC-V: Fix one bug for floating-point static frm > LGTM, thanks :) just a moment please, I still wanted to reply ;) Regards Robin
Re: [PATCH v4] RISC-V: Fix one bug for floating-point static frm
On Wed, Jul 5, 2023 at 3:12 PM Robin Dapp via Gcc-patches wrote: > > > LGTM, thanks :) > > just a moment please, I still wanted to reply ;) Sure :) > > Regards > Robin >
Re: [PATCH v4] RISC-V: Fix one bug for floating-point static frm
> LGTM, thanks :) just a moment please, I still wanted to reply ;) Regards Robin
Re: [PATCH v4] RISC-V: Fix one bug for floating-point static frm
LGTM, thanks :)
On Wed, Jul 5, 2023 at 3:03 PM Pan Li via Gcc-patches
wrote:
>
> From: Pan Li
>
> 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
>
> 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.
> (riscv_mode_after): Likewise for after mode.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/riscv/rvv/base/float-point-frm-insert-6.c: New test.
> ---
> gcc/config/riscv/riscv.cc | 16 +++---
> .../riscv/rvv/base/float-point-frm-insert-6.c | 31 +++
> 2 files changed, 42 insertions(+), 5 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..4db32de5696 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,9 @@ 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;
> + /* According to RVV 1.0 spec, all vector floating-point operations use
> +the dynamic rounding mode in the frm register. */
> + return code >= 0 ? get_attr_frm_mode (insn) : FRM_MODE_DYN;
> default:
>gcc_unreachable ();
> }
> @@ -7757,7 +7759,7 @@ riscv_mode_after (int entity, int mode, rtx_insn *insn)
> case RISCV_FRM:
>return riscv_entity_mode_after (FRM_REGNUM, insn, mode,
> (int (*)(rtx_insn *)) get_attr_frm_mode,
> - FRM_MODE_NONE);
> + FRM_MODE_DYN);
> default:
>gcc_unreachable ();
> }
> @@ -7774,7 +7776,9 @@ riscv_mode_entry (int entity)
> case RISCV_VXRM:
>return VXRM_MODE_NONE;
> case RISCV_FRM:
> - return FRM_MODE_NONE;
> + /* According to RVV 1.0 spec, all vector floating-point operations use
> +the dynamic rounding mode in the frm register. */
> + return FRM_MODE_DYN;
> default:
>gcc_unreachable ();
> }
> @@ -7791,7 +7795,9 @@ riscv_mode_exit (int entity)
> case RISCV_VXRM:
>return VXRM_MODE_NONE;
> case RISCV_FRM:
> - return FRM_MODE_NONE;
> + /* According to RVV 1.0 spec, all vector floating-point operations use
> +the dynamic rounding mode in the frm register. */
> + 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 000..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
>
