Compared to the patch v2, I added Zfinx check and Zfh check. Please help to
review again.
Thanks,
Zhijin
>From 9ddb402cebe868050ebc2f75e4d87238161411b4 Mon Sep 17 00:00:00 2001
From: Zhijin Zeng <zhijin.z...@spacemit.com>
Date: Sat, 11 Jan 2025 12:09:11 +0800
Subject: [PATCH] RISC-V: Fix mode compatibility between floating-point and
integer value
I find there are some unnecessary fmv instructions in glibc math exp,
and reduce exp function to the attached test case. The unnecessary
fmv instructions as follow:
```
fld fa4,16(a4)
fmadd.d fa2,fa2,fa0,fa5
fld fa0,56(a4)
fmv.x.d a5,fa2 *****
fld fa2,48(a4)
fmv.d.x fa1,a5 *****
andi a3,a5,127
addi a3,a3,15
fsub.d fa5,fa1,fa5
slli a3,a3,3
```
The data of fa2 and fa1 are the same, we should directly use fa2
rather than fa1 in following instructions and save one fmv instruction.
The `fmv.d.x a5,fa2` is correspond to pattern `(subreg:DI (reg/v:DF 143`.
In ira pass, virtual register r143 is assigned to GP_REGS, so its data
need be copied to FP_REGS before `fsub.d fa5,fa1,fa5` by reload pass,
and that's exactly the `fmv.d.x fa1,a5` instruction.
To fix this, we need to change the 4 functions.
1. FP_REGS can't be used for virtual register 143 in `(subreg:DI (reg/v:DF 143`.
I change riscv_hard_regno_mode_ok and riscv_can_change_mode_class to
support it. RISC-V fmv variant instructions can be used to move data
between FP_REGS and GR_REGS, and FP_REGS are compatible with DImode and
SImode.
2. The fwprop1 pass will fold the `(subreg:DI (reg/v:DF 143` and make
the cost calcutation method get incorrect cost and still assign r143 to
GP_REGS. Actually FP_REGS and GR_REGS are not tieable without Zfinx
extension, and need to fix riscv_modes_tieable_p.
3. JALR_REGS is also a subset of GR_REGS and need to be taken
into acount in riscv_register_move_cost, otherwise it will get
a incorrect cost.
gcc/ChangeLog:
* config/riscv/riscv.cc (riscv_register_move_cost):
(riscv_hard_regno_mode_ok):
(riscv_modes_tieable_p):
(riscv_can_change_mode_class):
gcc/testsuite/ChangeLog:
* gcc.target/riscv/fwprop1-modes-tieable.c: New test.
---
gcc/config/riscv/riscv.cc | 39 ++++++++-
.../gcc.target/riscv/fwprop1-modes-tieable.c | 82 +++++++++++++++++++
2 files changed, 118 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/fwprop1-modes-tieable.c
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 65e09842fde..cd1faeb035d 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -9588,9 +9588,10 @@ riscv_register_move_cost (machine_mode mode,
reg_class_t from, reg_class_t to)
{
bool from_is_fpr = from == FP_REGS || from == RVC_FP_REGS;
- bool from_is_gpr = from == GR_REGS || from == RVC_GR_REGS;
+ bool from_is_gpr = from == GR_REGS || from == RVC_GR_REGS
+ || from == JALR_REGS;
bool to_is_fpr = to == FP_REGS || to == RVC_FP_REGS;
- bool to_is_gpr = to == GR_REGS || to == RVC_GR_REGS;
+ bool to_is_gpr = to == GR_REGS || to == RVC_GR_REGS || to == JALR_REGS;
if ((from_is_fpr && to == to_is_gpr) ||
(from_is_gpr && to_is_fpr))
return tune_param->fmv_cost;
@@ -9696,7 +9697,10 @@ riscv_hard_regno_mode_ok (unsigned int regno,
machine_mode mode)
return false;
if (GET_MODE_CLASS (mode) != MODE_FLOAT
- && GET_MODE_CLASS (mode) != MODE_COMPLEX_FLOAT)
+ && GET_MODE_CLASS (mode) != MODE_COMPLEX_FLOAT
+ && !((mode == DImode && TARGET_DOUBLE_FLOAT && TARGET_64BIT)
+ || (mode == SImode && TARGET_HARD_FLOAT)
+ || (mode == HImode && TARGET_ZFHMIN)))
return false;
/* Only use callee-saved registers if a potential callee is guaranteed
@@ -9753,6 +9757,12 @@ riscv_modes_tieable_p (machine_mode mode1, machine_mode
mode2)
E.g. V2SI and DI are not tieable. */
if (riscv_v_ext_mode_p (mode1) != riscv_v_ext_mode_p (mode2))
return false;
+ if (!TARGET_ZFINX
+ && ((GET_MODE_CLASS (mode1) == MODE_FLOAT
+ && GET_MODE_CLASS (mode2) == MODE_INT)
+ || (GET_MODE_CLASS (mode2) == MODE_FLOAT
+ && GET_MODE_CLASS (mode1) == MODE_INT)))
+ return false;
return (mode1 == mode2
|| !(GET_MODE_CLASS (mode1) == MODE_FLOAT
&& GET_MODE_CLASS (mode2) == MODE_FLOAT));
@@ -11160,7 +11170,30 @@ riscv_can_change_mode_class (machine_mode from,
machine_mode to,
&& riscv_v_ext_vls_mode_p (from)
&& !ordered_p (BITS_PER_RISCV_VECTOR, GET_MODE_PRECISION (from)))
return false;
+ if ((SCALAR_INT_MODE_P (to) || SCALAR_FLOAT_MODE_P (to))
+ && (rclass == FP_REGS || rclass == RVC_FP_REGS
+ || rclass == GR_REGS || rclass == RVC_GR_REGS
+ || rclass == JALR_REGS))
+ {
+ /* fmv.x.w or fmv.w.x. */
+ if (TARGET_HARD_FLOAT)
+ if ((from == SFmode && to == SImode)
+ || (from == SImode && to == SFmode))
+ return true;
+ /* fmv.x.d or fmv.d.x. */
+ if (TARGET_DOUBLE_FLOAT && TARGET_64BIT)
+ if ((from == DFmode && to == DImode)
+ || (from == DImode && to == DFmode))
+ return true;
+
+ /* fmv.x.h or fmv.h.x. */
+ if (TARGET_ZFHMIN)
+ if ((from == HFmode && to == HImode)
+ || (from == HImode && to == HFmode))
+ return true;
+
+ }
return !reg_classes_intersect_p (FP_REGS, rclass);
}
diff --git a/gcc/testsuite/gcc.target/riscv/fwprop1-modes-tieable.c
b/gcc/testsuite/gcc.target/riscv/fwprop1-modes-tieable.c
new file mode 100644
index 00000000000..54212ec8376
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/fwprop1-modes-tieable.c
@@ -0,0 +1,82 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc -mabi=lp64d -fdump-rtl-fwprop1" } */
+/* { dg-skip-if "" { *-*-* } {"-Os" "-O1" "-O0" "-Og" "-Oz" "-flto"} } */
+/* { dg-final { scan-assembler-times "fmv.d.x" 1 } } */
+/* { dg-final { scan-rtl-dump-not
"\\(and:DI^(\s|\n)?$\\(subreg:DI^(\s|\n)?$\\(reg/v:DF" "fwprop1" } } */
+
+#include <stdint.h>
+
+#define EXP_TABLE_BITS 7
+#define EXP_POLY_ORDER 5
+#define EXP2_POLY_ORDER 5
+struct exp_data
+{
+ double invln2N;
+ double shift;
+ double negln2hiN;
+ double negln2loN;
+ double poly[4]; /* Last four coefficients. */
+ double exp2_shift;
+ double exp2_poly[EXP2_POLY_ORDER];
+ uint64_t tab[2*(1 << EXP_TABLE_BITS)];
+};
+
+extern struct exp_data __exp_data;
+
+#define N (1 << EXP_TABLE_BITS)
+#define InvLn2N __exp_data.invln2N
+#define NegLn2hiN __exp_data.negln2hiN
+#define NegLn2loN __exp_data.negln2loN
+#define Shift __exp_data.shift
+#define T __exp_data.tab
+#define C2 __exp_data.poly[5 - EXP_POLY_ORDER]
+#define C3 __exp_data.poly[6 - EXP_POLY_ORDER]
+#define C4 __exp_data.poly[7 - EXP_POLY_ORDER]
+#define C5 __exp_data.poly[8 - EXP_POLY_ORDER]
+
+static inline uint64_t
+asuint64 (double f)
+{
+ union
+ {
+ double f;
+ uint64_t i;
+ } u = {f};
+ return u.i;
+}
+
+static inline double
+asdouble (uint64_t i)
+{
+ union
+ {
+ uint64_t i;
+ double f;
+ } u = {i};
+ return u.f;
+}
+
+double
+__testexp (double x)
+{
+ uint64_t ki, idx, sbits, top;
+ double kd, z, r, scale, tmp;
+
+ z = InvLn2N * x;
+
+ kd = z + Shift;
+ ki = asuint64 (kd);
+ kd -= Shift;
+
+ r = kd * NegLn2hiN + kd * NegLn2loN;
+
+ idx = (ki % N);
+ top = ki << (52 - EXP_TABLE_BITS);
+
+ sbits = T[idx + 1] + top;
+
+ tmp = C4 + r * C5;
+
+ scale = asdouble (sbits);
+ return scale * tmp;
+}
--
2.25.1
> From: "Zhijin Zeng"<zhijin.z...@spacemit.com>
> Date: Sat, Jan 11, 2025, 19:28
> Subject: Re: [PATCH v2] RISC-V: Fix riscv_modes_tieable_p
> To: "Jeff Law"<jeffreya...@gmail.com>
> Cc: "Palmer Dabbelt"<pal...@dabbelt.com>, "Robin Dapp"<rdapp....@gmail.com>,
> "gcc-patches"<gcc-patches@gcc.gnu.org>, "Kito Cheng"<kito.ch...@gmail.com>,
> "richard.sandiford"<richard.sandif...@linaro.org>
> I'm so sorry that I didn't describe the patch clearly. I refactored the patch
> and added some new changes. Initially I split them into two patches, which
> is probably not right.
>
> Thanks,
> Zhijin
>
> From ca072f040c876df4117f475eeb74c7eb8882bed8 Mon Sep 17 00:00:00 2001
> From: Zhijin Zeng <zhijin.z...@spacemit.com>
> Date: Sat, 11 Jan 2025 12:09:11 +0800
> Subject: [PATCH] RISC-V: Fix mode compatibility between floating-point and
> integer value
>
> I find there are some unnecessary fmv instructions in glibc math exp,
> and reduce exp function to the attached test case. The unnecessary
> fmv instructions as follow:
>
> ```
> fld fa4,16(a4)
> fmadd.d fa2,fa2,fa0,fa5
> fld fa0,56(a4)
> fmv.x.d a5,fa2 *****
> fld fa2,48(a4)
> fmv.d.x fa1,a5 *****
> andi a3,a5,127
> addi a3,a3,15
> fsub.d fa5,fa1,fa5
> slli a3,a3,3
> ```
>
> The data of fa2 and fa1 are the same, we should directly use fa2
> rather than fa1 in following instructions and save one fmv instruction.
>
> The `fmv.d.x a5,fa2` is correspond to pattern `(subreg:DI (reg/v:DF 143`.
> In ira pass, virtual register r143 is assigned to GP_REGS, so its data
> need be copied to FP_REGS before `fsub.d fa5,fa1,fa5` by reload pass,
> and that's exactly the `fmv.d.x fa1,a5` instruction.
>
> To fix this, we need to change the 4 functions.
>
> 1. FP_REGS can't be used for virtual register 143 in `(subreg:DI (reg/v:DF
> 143`.
> I change riscv_hard_regno_mode_ok and riscv_can_change_mode_class to
> support it. RISC-V fmv variant instructions can be used to move data
> between FP_REGS and GR_REGS, and FP_REGS are compatible with DImode and
> SImode.
>
> 2. The fwprop1 pass will fold the `(subreg:DI (reg/v:DF 143` and make
> the cost calcutation method get incorrect cost and still assign r143 to
> GP_REGS. Actually FP_REGS and GR_REGS are not tieable without Zfinx
> extension, and need to fix riscv_modes_tieable_p.
>
> 3. JALR_REGS is also a subset of GR_REGS and need to be taken
> into acount in riscv_register_move_cost, otherwise it will get
> a incorrect cost.
>
> gcc/ChangeLog:
>
> * config/riscv/riscv.cc<http://riscv.cc> (riscv_register_move_cost):
> (riscv_hard_regno_mode_ok):
> (riscv_modes_tieable_p):
> (riscv_can_change_mode_class):
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/riscv/fwprop1-modes-tieable.c: New test.
> ---
> gcc/config/riscv/riscv.cc<http://riscv.cc> | 31 ++++++-
> .../gcc.target/riscv/fwprop1-modes-tieable.c | 82 +++++++++++++++++++
> 2 files changed, 110 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/fwprop1-modes-tieable.c
>
> diff --git a/gcc/config/riscv/riscv.cc<http://riscv.cc>
> b/gcc/config/riscv/riscv.cc<http://riscv.cc>
> index 65e09842fde..8a8444d6b92 100644
> --- a/gcc/config/riscv/riscv.cc<http://riscv.cc>
> +++ b/gcc/config/riscv/riscv.cc<http://riscv.cc>
> @@ -9588,9 +9588,10 @@ riscv_register_move_cost (machine_mode mode,
> reg_class_t from, reg_class_t to)
> {
> bool from_is_fpr = from == FP_REGS || from == RVC_FP_REGS;
> - bool from_is_gpr = from == GR_REGS || from == RVC_GR_REGS;
> + bool from_is_gpr = from == GR_REGS || from == RVC_GR_REGS
> + || from == JALR_REGS;
> bool to_is_fpr = to == FP_REGS || to == RVC_FP_REGS;
> - bool to_is_gpr = to == GR_REGS || to == RVC_GR_REGS;
> + bool to_is_gpr = to == GR_REGS || to == RVC_GR_REGS || to == JALR_REGS;
> if ((from_is_fpr && to == to_is_gpr) ||
> (from_is_gpr && to_is_fpr))
> return tune_param->fmv_cost;
> @@ -9696,7 +9697,10 @@ riscv_hard_regno_mode_ok (unsigned int regno,
> machine_mode mode)
> return false;
>
> if (GET_MODE_CLASS (mode) != MODE_FLOAT
> - && GET_MODE_CLASS (mode) != MODE_COMPLEX_FLOAT)
> + && GET_MODE_CLASS (mode) != MODE_COMPLEX_FLOAT
> + && GET_MODE_CLASS (mode) != MODE_COMPLEX_FLOAT
> + && !((mode == DImode && TARGET_DOUBLE_FLOAT && TARGET_64BIT)
> + || (mode == SImode && TARGET_HARD_FLOAT)))
> return false;
>
> /* Only use callee-saved registers if a potential callee is guaranteed
> @@ -9753,6 +9757,11 @@ riscv_modes_tieable_p (machine_mode mode1,
> machine_mode mode2)
> E.g. V2SI and DI are not tieable. */
> if (riscv_v_ext_mode_p (mode1) != riscv_v_ext_mode_p (mode2))
> return false;
> + if ((GET_MODE_CLASS (mode1) == MODE_FLOAT
> + && GET_MODE_CLASS (mode2) == MODE_INT)
> + || (GET_MODE_CLASS (mode2) == MODE_FLOAT
> + && GET_MODE_CLASS (mode1) == MODE_INT))
> + return false;
> return (mode1 == mode2
> || !(GET_MODE_CLASS (mode1) == MODE_FLOAT
> && GET_MODE_CLASS (mode2) == MODE_FLOAT));
> @@ -11160,7 +11169,23 @@ riscv_can_change_mode_class (machine_mode from,
> machine_mode to,
> && riscv_v_ext_vls_mode_p (from)
> && !ordered_p (BITS_PER_RISCV_VECTOR, GET_MODE_PRECISION (from)))
> return false;
> + if ((SCALAR_INT_MODE_P (to) || SCALAR_FLOAT_MODE_P (to))
> + && (rclass == FP_REGS || rclass == RVC_FP_REGS
> + || rclass == GR_REGS || rclass == RVC_GR_REGS
> + || rclass == JALR_REGS))
> + {
> + /* fmv.x.w or fmv.w.x. */
> + if (TARGET_HARD_FLOAT)
> + if ((from == SFmode && to == SImode)
> + || (from == SImode && to == SFmode))
> + return true;
>
> + /* fmv.x.d or fmv.d.x. */
> + if (TARGET_DOUBLE_FLOAT && TARGET_64BIT)
> + if ((from == DFmode && to == DImode)
> + || (from == DImode && to == DFmode))
> + return true;
> + }
> return !reg_classes_intersect_p (FP_REGS, rclass);
> }
>
> diff --git a/gcc/testsuite/gcc.target/riscv/fwprop1-modes-tieable.c
> b/gcc/testsuite/gcc.target/riscv/fwprop1-modes-tieable.c
> new file mode 100644
> index 00000000000..54212ec8376
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/fwprop1-modes-tieable.c
> @@ -0,0 +1,82 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gc -mabi=lp64d -fdump-rtl-fwprop1" } */
> +/* { dg-skip-if "" { *-*-* } {"-Os" "-O1" "-O0" "-Og" "-Oz" "-flto"} } */
> +/* { dg-final { scan-assembler-times "fmv.d.x" 1 } } */
> +/* { dg-final { scan-rtl-dump-not
> "\\(and:DI^(\s|\n)?$\\(subreg:DI^(\s|\n)?$\\(reg/v:DF" "fwprop1" } } */
> +
> +#include <stdint.h>
> +
> +#define EXP_TABLE_BITS 7
> +#define EXP_POLY_ORDER 5
> +#define EXP2_POLY_ORDER 5
> +struct exp_data
> +{
> + double invln2N;
> + double shift;
> + double negln2hiN;
> + double negln2loN;
> + double poly[4]; /* Last four coefficients. */
> + double exp2_shift;
> + double exp2_poly[EXP2_POLY_ORDER];
> + uint64_t tab[2*(1 << EXP_TABLE_BITS)];
> +};
> +
> +extern struct exp_data __exp_data;
> +
> +#define N (1 << EXP_TABLE_BITS)
> +#define InvLn2N __exp_data.invln2N
> +#define NegLn2hiN __exp_data.negln2hiN
> +#define NegLn2loN __exp_data.negln2loN
> +#define Shift __exp_data.shift
> +#define T __exp_data.tab
> +#define C2 __exp_data.poly[5 - EXP_POLY_ORDER]
> +#define C3 __exp_data.poly[6 - EXP_POLY_ORDER]
> +#define C4 __exp_data.poly[7 - EXP_POLY_ORDER]
> +#define C5 __exp_data.poly[8 - EXP_POLY_ORDER]
> +
> +static inline uint64_t
> +asuint64 (double f)
> +{
> + union
> + {
> + double f;
> + uint64_t i;
> + } u = {f};
> + return u.i;
> +}
> +
> +static inline double
> +asdouble (uint64_t i)
> +{
> + union
> + {
> + uint64_t i;
> + double f;
> + } u = {i};
> + return u.f;
> +}
> +
> +double
> +__testexp (double x)
> +{
> + uint64_t ki, idx, sbits, top;
> + double kd, z, r, scale, tmp;
> +
> + z = InvLn2N * x;
> +
> + kd = z + Shift;
> + ki = asuint64 (kd);
> + kd -= Shift;
> +
> + r = kd * NegLn2hiN + kd * NegLn2loN;
> +
> + idx = (ki % N);
> + top = ki << (52 - EXP_TABLE_BITS);
> +
> + sbits = T[idx + 1] + top;
> +
> + tmp = C4 + r * C5;
> +
> + scale = asdouble (sbits);
> + return scale * tmp;
> +}
> --
> 2.25.1
>
>
> > From: "Jeff Law"<jeffreya...@gmail.com>
> > Date: Sat, Jan 11, 2025, 08:06
> > Subject: Re: [PATCH] RISC-V: Fix riscv_modes_tieable_p
> > To: "Palmer Dabbelt"<pal...@dabbelt.com>
> > Cc: "Robin Dapp"<rdapp....@gmail.com>, <zhijin.z...@spacemit.com>,
> > <gcc-patches@gcc.gnu.org>, "Kito Cheng"<kito.ch...@gmail.com>,
> > <richard.sandif...@linaro.org>
> > On 1/10/25 4:59 PM, Palmer Dabbelt wrote:
>
> > > On Fri, 10 Jan 2025 12:21:15 PST (-0800), jeffreya...@gmail.com wrote:
>
> > >>
> > >>
> > >> On 1/10/25 12:11 PM, Robin Dapp wrote:
>
> > >>>> Integer values and floating-point values need to be converted
>
> > >>>> by fmv series instructions. So if mode1 is MODE_INT and mode2
>
> > >>>> is MODE_FLOAT, we should return false in riscv_modes_tieable_p,
>
> > >>>> and vice versa.
>
> > >>>
> > >>> I think that's on purpose because we can read and write float values
>
> > >>> from/to integer registers. Maybe it's a cost problem that we spill
>
> > >>> at some point rather than access directly?
>
> > >> But even if you spill, as long as loads/stores don't modify the value
>
> > >> then I think we're OK from a correctness standpoint.
>
> > >>
> > >>
> > >>>
> > >>> If I compile your test case I do see converting moves in the final
>
> > >>> assembly - is there something you're concerned about in particular?
>
> > >
>
> > > Which appears to be the glibc code (or very similar to it), and I don't
>
> > > think we've had users reporting incorrect results there.
>
> > There's certainly cases in glibc that very much want to just move a blob
>
> > of data from an FP register into a GPR without any kind of interpretation.
>
> >
> > >
>
> > >> Which was my general question as well. Under precisely what
>
> > >> circumstances is this causing a problem? The secondary question would
>
> > >> be how does this change interact with the finx and related extensions?
>
> > >
>
> > > FWIW I'm also a bit lost here: I'd expect riscv_hard_regno_mode_ok() to
>
> > > be sufficient to handle these X/F register mixing cases, and thus us not
>
> > > to need any more special handling in riscv_modes_tieable_p().
>
> > >
>
> > > (I think we're safe for finx with the current code, as we can access the
>
> > > registers safely there.)
>
> > >
>
> > > So maybe there's something else also needed to trigger this?
>
> > I don't think you're lost, Robin and I have similar concerns as yours.
>
> > At this point I don't think the patch is right/correct, but I'm also
>
> > open to the possibility there's something more complex going on that
>
> > hasn't been fully explained. Hence my request for an explanation of the
>
> > precise circumstances when Zhijin thinks this is necessary.
>
> >
> >
> > Jeff
> >
This message and any attachment are confidential and may be privileged or
otherwise protected from disclosure. If you are not an intended recipient of
this message, please delete it and any attachment from your system and notify
the sender immediately by reply e-mail. Unintended recipients should not use,
copy, disclose or take any action based on this message or any information
contained in this message. Emails cannot be guaranteed to be secure or error
free as they can be intercepted, amended, lost or destroyed, and you should
take full responsibility for security checking.
本邮件及其任何附件具有保密性质，并可能受其他保护或不允许被披露给第三方。如阁下误收到本邮件，敬请立即以回复电子邮件的方式通知发件人，并将本邮件及其任何附件从阁下系统中予以删除。如阁下并非本邮件写明之收件人，敬请切勿使用、复制、披露本邮件或其任何内容，亦请切勿依本邮件或其任何内容而采取任何行动。电子邮件无法保证是一种安全和不会出现任何差错的通信方式，可能会被拦截、修改、丢失或损坏，收件人需自行负责做好安全检查。
