Re: [PATCH 2/2] RISC-V: Move mode assertion out of conditional branch in emit_insn

2024-06-12 Thread Edwin Lu 

On 6/12/2024 12:39 AM, Robin Dapp wrote:

Hi Edwin,

this LGTM but I just remembered I intended to turn the assert
into a more descriptive error.

The attached patch has been sitting on my local branch for a
while.  Maybe we should rather fold yours into it?


That's fine with me! Having more descriptive errors would have helped a 
lot during the triaging/debugging step :)


Edwin


Regards
  Robin

 From d164403ef577917f905c1690f2199fab330f05e2 Mon Sep 17 00:00:00 2001
From: Robin Dapp 
Date: Fri, 31 May 2024 14:51:17 +0200
Subject: [PATCH] RISC-V: Use descriptive errors instead of asserts.

In emit_insn we forestall possible ICEs in maybe_legitimize_operand by
asserting.  This patch replaces the assertions by more descriptive
internal errors.

gcc/ChangeLog:

* config/riscv/riscv-v.cc: Replace asserts by internal errors.
---
  gcc/config/riscv/riscv-v.cc | 27 ++-
  1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
index 8911f5783c8..810203b8ba5 100644
--- a/gcc/config/riscv/riscv-v.cc
+++ b/gcc/config/riscv/riscv-v.cc
@@ -50,6 +50,7 @@
  #include "rtx-vector-builder.h"
  #include "targhooks.h"
  #include "predict.h"
+#include "errors.h"
  
  using namespace riscv_vector;
  
@@ -291,10 +292,20 @@ public:

if (mode == VOIDmode)
  mode = Pmode;
else
- /* Early assertion ensures same mode since maybe_legitimize_operand
-will check this.  */
- gcc_assert (GET_MODE (ops[opno]) == VOIDmode
- || GET_MODE (ops[opno]) == mode);
+ {
+   /* Early assertion ensures same mode since maybe_legitimize_operand
+  will check this.  */
+   machine_mode required_mode = GET_MODE (ops[opno]);
+   if (required_mode != VOIDmode && required_mode != mode)
+ {
+   internal_error ("expected mode %s for operand %d of "
+   "insn %s but got mode %s.\n",
+   GET_MODE_NAME (mode),
+   opno,
+   insn_data[(int) icode].name,
+   GET_MODE_NAME (required_mode));
+ }
+ }
  
  	add_input_operand (ops[opno], mode);

}
@@ -346,7 +357,13 @@ public:
  else if (m_insn_flags & VXRM_RDN_P)
add_rounding_mode_operand (VXRM_RDN);
  
-gcc_assert (insn_data[(int) icode].n_operands == m_opno);

+
+if (insn_data[(int) icode].n_operands != m_opno)
+  internal_error ("invalid number of operands for insn %s, "
+ "expected %d but got %d.\n",
+ insn_data[(int) icode].name,
+ insn_data[(int) icode].n_operands, m_opno);
+
  expand (icode, any_mem_p);
}

Re: [PATCH 2/2] RISC-V: Move mode assertion out of conditional branch in emit_insn

2024-06-12 Thread Robin Dapp 
Hi Edwin,

this LGTM but I just remembered I intended to turn the assert
into a more descriptive error.

The attached patch has been sitting on my local branch for a
while.  Maybe we should rather fold yours into it?

Regards
 Robin

>From d164403ef577917f905c1690f2199fab330f05e2 Mon Sep 17 00:00:00 2001
From: Robin Dapp 
Date: Fri, 31 May 2024 14:51:17 +0200
Subject: [PATCH] RISC-V: Use descriptive errors instead of asserts.

In emit_insn we forestall possible ICEs in maybe_legitimize_operand by
asserting.  This patch replaces the assertions by more descriptive
internal errors.

gcc/ChangeLog:

* config/riscv/riscv-v.cc: Replace asserts by internal errors.
---
 gcc/config/riscv/riscv-v.cc | 27 ++-
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
index 8911f5783c8..810203b8ba5 100644
--- a/gcc/config/riscv/riscv-v.cc
+++ b/gcc/config/riscv/riscv-v.cc
@@ -50,6 +50,7 @@
 #include "rtx-vector-builder.h"
 #include "targhooks.h"
 #include "predict.h"
+#include "errors.h"
 
 using namespace riscv_vector;
 
@@ -291,10 +292,20 @@ public:
if (mode == VOIDmode)
  mode = Pmode;
else
- /* Early assertion ensures same mode since maybe_legitimize_operand
-will check this.  */
- gcc_assert (GET_MODE (ops[opno]) == VOIDmode
- || GET_MODE (ops[opno]) == mode);
+ {
+   /* Early assertion ensures same mode since maybe_legitimize_operand
+  will check this.  */
+   machine_mode required_mode = GET_MODE (ops[opno]);
+   if (required_mode != VOIDmode && required_mode != mode)
+ {
+   internal_error ("expected mode %s for operand %d of "
+   "insn %s but got mode %s.\n",
+   GET_MODE_NAME (mode),
+   opno,
+   insn_data[(int) icode].name,
+   GET_MODE_NAME (required_mode));
+ }
+ }
 
add_input_operand (ops[opno], mode);
   }
@@ -346,7 +357,13 @@ public:
 else if (m_insn_flags & VXRM_RDN_P)
   add_rounding_mode_operand (VXRM_RDN);
 
-gcc_assert (insn_data[(int) icode].n_operands == m_opno);
+
+if (insn_data[(int) icode].n_operands != m_opno)
+  internal_error ("invalid number of operands for insn %s, "
+ "expected %d but got %d.\n",
+ insn_data[(int) icode].name,
+ insn_data[(int) icode].n_operands, m_opno);
+
 expand (icode, any_mem_p);
   }
 
-- 
2.45.1

[PATCH 2/2] RISC-V: Move mode assertion out of conditional branch in emit_insn

2024-06-11 Thread Edwin Lu 
When emitting insns, we have an early assertion to ensure the input
operand's mode and the expanded operand's mode are the same; however, it
does not perform this check if the pattern does not have an explicit
machine mode specifying the operand. In this scenario, it will always
assume that mode = Pmode to correctly satisfy the
maybe_legitimize_operand check, however, there may be problems when
working in 32 bit environments.

Make the assert unconditional

gcc/ChangeLog:

* config/riscv/riscv-v.cc: Move assert out of conditional block

Signed-off-by: Edwin Lu 
---
 gcc/config/riscv/riscv-v.cc | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
index 8911f5783c8..6387727833f 100644
--- a/gcc/config/riscv/riscv-v.cc
+++ b/gcc/config/riscv/riscv-v.cc
@@ -290,11 +290,11 @@ public:
   always Pmode.  */
if (mode == VOIDmode)
  mode = Pmode;
-   else
- /* Early assertion ensures same mode since maybe_legitimize_operand
-will check this.  */
- gcc_assert (GET_MODE (ops[opno]) == VOIDmode
- || GET_MODE (ops[opno]) == mode);
+
+   /* Early assertion ensures same mode since maybe_legitimize_operand
+  will check this.  */
+   gcc_assert (GET_MODE (ops[opno]) == VOIDmode
+ || GET_MODE (ops[opno]) == mode);
 
add_input_operand (ops[opno], mode);
   }
-- 
2.34.1