diff mbox series

RISC-V: xtheadfmemidx: Disable if xtheadmemidx is not available

Message ID 20231205151631.3202932-1-christoph.muellner@vrull.eu
State Unresolved
Headers
Series RISC-V: xtheadfmemidx: Disable if xtheadmemidx is not available |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Christoph Müllner Dec. 5, 2023, 3:16 p.m. UTC 
  XTheadMemIdx provides register-register offsets for GP register
loads/stores.  XTheadFMemIdx does the same for FP registers.

We've observed an issue with XTheadFMemIdx-only builds, where FP
registers have been promoted to GP registers:

(insn 26 22 51 (set (reg:DF 15 a5 [orig:136 <retval> ] [136])
        (mem/u:DF (plus:DI (reg/f:DI 15 a5 [141])
                (reg:DI 10 a0 [144])) [1 CSWTCH.2[_10]+0 S8 A64])) 217 {*movdf_hardfloat_rv64}
     (expr_list:REG_DEAD (reg:DI 10 a0 [144])
        (nil)))

This results in the following assembler error:
  Assembler messages:
  Error: unrecognized opcode `th.lrd a5,a5,a0,0', extension `xtheadmemidx' required

There seems to be a (reasonable) assumption, that addressing modes
for FP registers are compatible with those of GP registers.

We already ran into a similar issue during development of the
XTheadFMemIdx support patch, where we could trace the issue down to
the optimization splitters.  Back then we simply disabled them in case
XTheadMemIdx is not available.  But as it turned out, that was not
enough.

To ensure, we won't see such issues anymore, let's make the support
for XTheadFMemIdx depend on XTheadMemIdx.  I.e., if only XTheadFMemIdx
is available, then no instructions of this extension will be emitted.

While this looks a bit drastic at first view, it is the best practical
solution since XTheadFMemIdx without XTheadMemIdx does not exist in real
hardware and would be an odd thing to do.

gcc/ChangeLog:

	* config/riscv/thead.cc (th_memidx_classify_address_index):
	Require TARGET_XTHEADMEMIDX for FP modes.
	* config/riscv/thead.md: Require TARGET_XTHEADMEMIDX for all
	XTheadFMemIdx pattern.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/xtheadfmemidx-without-xtheadmemidx.c: New test.

Reported-by: Jin Ma <jinma@linux.alibaba.com>
Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
---
 gcc/config/riscv/thead.cc                     |  3 +-
 gcc/config/riscv/thead.md                     | 19 ++++-----
 .../xtheadfmemidx-without-xtheadmemidx.c      | 39 +++++++++++++++++++
 3 files changed, 51 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/xtheadfmemidx-without-xtheadmemidx.c

Comments

Jeff Law Dec. 6, 2023, 9:28 p.m. UTC | #1 
On 12/5/23 08:16, Christoph Müllner wrote:
> XTheadMemIdx provides register-register offsets for GP register
> loads/stores.  XTheadFMemIdx does the same for FP registers.
> 
> We've observed an issue with XTheadFMemIdx-only builds, where FP
> registers have been promoted to GP registers:
> 
> (insn 26 22 51 (set (reg:DF 15 a5 [orig:136 <retval> ] [136])
>          (mem/u:DF (plus:DI (reg/f:DI 15 a5 [141])
>                  (reg:DI 10 a0 [144])) [1 CSWTCH.2[_10]+0 S8 A64])) 217 {*movdf_hardfloat_rv64}
>       (expr_list:REG_DEAD (reg:DI 10 a0 [144])
>          (nil)))
> 
> This results in the following assembler error:
>    Assembler messages:
>    Error: unrecognized opcode `th.lrd a5,a5,a0,0', extension `xtheadmemidx' required
> 
> There seems to be a (reasonable) assumption, that addressing modes
> for FP registers are compatible with those of GP registers.
> 
> We already ran into a similar issue during development of the
> XTheadFMemIdx support patch, where we could trace the issue down to
> the optimization splitters.  Back then we simply disabled them in case
> XTheadMemIdx is not available.  But as it turned out, that was not
> enough.
> 
> To ensure, we won't see such issues anymore, let's make the support
> for XTheadFMemIdx depend on XTheadMemIdx.  I.e., if only XTheadFMemIdx
> is available, then no instructions of this extension will be emitted.
> 
> While this looks a bit drastic at first view, it is the best practical
> solution since XTheadFMemIdx without XTheadMemIdx does not exist in real
> hardware and would be an odd thing to do.
> 
> gcc/ChangeLog:
> 
> 	* config/riscv/thead.cc (th_memidx_classify_address_index):
> 	Require TARGET_XTHEADMEMIDX for FP modes.
> 	* config/riscv/thead.md: Require TARGET_XTHEADMEMIDX for all
> 	XTheadFMemIdx pattern.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/riscv/xtheadfmemidx-without-xtheadmemidx.c: New test.
OK.

Note that in the reload era this kind of issue was common.  Essentially 
you have a MEM, but you don't know if it's going to be used in a load or 
a store.  For loads you don't know if the destination will be a GPR, FPR 
or something else, similarly for stores you didn't know if the source 
value came from a GPR, FPR or elsewhere.

As a result you had to assume that you'd eventually see a GPR used with 
floating point modes and FPRs used with integer modes.  It caused all 
kinds of headaches on the PA.  Valid addressing modes differed across 
the various cases and integer multiplies actually used the FP unit and 
thus had to be moved into FP regs first made it even worse.

I think things are better with LRA, but I don't know if this class of 
problems is totally eliminated.    Point being I'm not surprised that 
you're seeing GPRs referenced in FP modes.

jeff
diff mbox series

Patch

diff --git a/gcc/config/riscv/thead.cc b/gcc/config/riscv/thead.cc
index bd9af7ecd60..20353995931 100644
--- a/gcc/config/riscv/thead.cc
+++ b/gcc/config/riscv/thead.cc
@@ -603,7 +603,8 @@  th_memidx_classify_address_index (struct riscv_address_info *info, rtx x,
 {
   /* Ensure that the mode is supported.  */
   if (!(TARGET_XTHEADMEMIDX && is_memidx_mode (mode))
-      && !(TARGET_XTHEADFMEMIDX && is_fmemidx_mode (mode)))
+      && !(TARGET_XTHEADMEMIDX
+	   && TARGET_XTHEADFMEMIDX && is_fmemidx_mode (mode)))
     return false;
 
   if (GET_CODE (x) != PLUS)
diff --git a/gcc/config/riscv/thead.md b/gcc/config/riscv/thead.md
index 2babfafb23c..186ca468875 100644
--- a/gcc/config/riscv/thead.md
+++ b/gcc/config/riscv/thead.md
@@ -822,11 +822,19 @@  (define_insn_and_split "*th_memidx_UZ_c"
 )
 
 ;; XTheadFMemIdx
+;; Note, that we might get GP registers in FP-mode (reg:DF a2)
+;; which cannot be handled by the XTheadFMemIdx instructions.
+;; This might even happend after register allocation.
+;; We could implement splitters that undo the combiner results
+;; if "after_reload && !HARDFP_REG_P (operands[0])", but this
+;; raises even more questions (e.g. split into what?).
+;; So let's solve this by simply requiring XTheadMemIdx
+;; which provides the necessary instructions to cover this case.
 
 (define_insn "*th_fmemidx_movsf_hardfloat"
   [(set (match_operand:SF 0 "nonimmediate_operand" "=f,th_m_mir,f,th_m_miu")
 	(match_operand:SF 1 "move_operand"         " th_m_mir,f,th_m_miu,f"))]
-  "TARGET_HARD_FLOAT && TARGET_XTHEADFMEMIDX
+  "TARGET_HARD_FLOAT && TARGET_XTHEADFMEMIDX && TARGET_XTHEADMEMIDX
    && (register_operand (operands[0], SFmode)
        || reg_or_0_operand (operands[1], SFmode))"
   { return riscv_output_move (operands[0], operands[1]); }
@@ -837,6 +845,7 @@  (define_insn "*th_fmemidx_movdf_hardfloat_rv64"
   [(set (match_operand:DF 0 "nonimmediate_operand" "=f,th_m_mir,f,th_m_miu")
 	(match_operand:DF 1 "move_operand"         " th_m_mir,f,th_m_miu,f"))]
   "TARGET_64BIT && TARGET_DOUBLE_FLOAT && TARGET_XTHEADFMEMIDX
+   && TARGET_XTHEADMEMIDX
    && (register_operand (operands[0], DFmode)
        || reg_or_0_operand (operands[1], DFmode))"
   { return riscv_output_move (operands[0], operands[1]); }
@@ -845,14 +854,6 @@  (define_insn "*th_fmemidx_movdf_hardfloat_rv64"
 
 ;; XTheadFMemIdx optimizations
 ;; Similar like XTheadMemIdx optimizations, but less cases.
-;; Note, that we might get GP registers in FP-mode (reg:DF a2)
-;; which cannot be handled by the XTheadFMemIdx instructions.
-;; This might even happend after register allocation.
-;; We could implement splitters that undo the combiner results
-;; if "after_reload && !HARDFP_REG_P (operands[0])", but this
-;; raises even more questions (e.g. split into what?).
-;; So let's solve this by simply requiring XTheadMemIdx
-;; which provides the necessary instructions to cover this case.
 
 (define_insn_and_split "*th_fmemidx_I_a"
   [(set (match_operand:TH_M_NOEXTF 0 "register_operand" "=f")
diff --git a/gcc/testsuite/gcc.target/riscv/xtheadfmemidx-without-xtheadmemidx.c b/gcc/testsuite/gcc.target/riscv/xtheadfmemidx-without-xtheadmemidx.c
new file mode 100644
index 00000000000..c5502390cca
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/xtheadfmemidx-without-xtheadmemidx.c
@@ -0,0 +1,39 @@ 
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-Og" } } */
+/* { dg-options "-march=rv64gc_xtheadfmemidx" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_xtheadfmemidx" { target { rv32 } } } */
+
+typedef union {
+  double v;
+  unsigned w;
+} my_t;
+
+double z;
+
+double foo (int i, int j)
+{
+
+  if (j)
+    {
+      switch (i)
+	{
+	case 0:
+	  return 1;
+	case 1:
+	  return 0;
+	case 2:
+	  return 3.0;
+	}
+    }
+
+  if (i == 1)
+    {
+      my_t u;
+      u.v = z;
+      u.w = 1;
+      z = u.v;
+    }
+  return z;
+}
+
+/* { dg-final { scan-assembler-not "th.lrd\t" } } */