[PATCH 0/2] Fix ICE with vwsll combine on 32bit targets

[PATCH 0/2] Fix ICE with vwsll combine on 32bit targets

[Edwin Lu]

The following testcases have been failing on rv32 targets since 
r15-953-gaf4bf422a69:
FAIL: gcc.target/riscv/rvv/autovec/binop/vwsll-1.c (internal compiler error: in maybe_legitimize_operand, at optabs.cc:8056)
FAIL: gcc.target/riscv/rvv/autovec/binop/vwsll-1.c (test for excess errors)

Fix the bug and also robustify our emit_insn by making an assertion
check unconditional

I'm not sure if this ICE warrants its own separate testcase since it is
already being tested. I do have a minimal testcase on hand if we would
like to add one.

Edwin Lu (2):
  RISC-V: Fix vwsll combine on rv32 targets
  RISC-V: Move mode assertion out of conditional branch in emit_insn

 gcc/config/riscv/autovec-opt.md |  1 +
 gcc/config/riscv/riscv-v.cc     | 10 +++++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 1/2] RISC-V: Fix vwsll combine on rv32 targets

[Edwin Lu]

On rv32 targets, vwsll_zext1_scalar_<mode> would trigger an ice in
maybe_legitimize_instruction when zero extending a uint32 to uint64 due
to a mismatch between the input operand's mode (DI) and the expanded insn
operand's mode (Pmode == SI). Ensure that mode of the operands match

gcc/ChangeLog:

	* config/riscv/autovec-opt.md: Fix mode mismatch

Signed-off-by: Edwin Lu <ewlu@rivosinc.com>
Co-authored-by: Robin Dapp <rdapp@ventanamicro.com>
---
 gcc/config/riscv/autovec-opt.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gcc/config/riscv/autovec-opt.md b/gcc/config/riscv/autovec-opt.md
index 6a2eabbd854..b9e5ccfef25 100644
--- a/gcc/config/riscv/autovec-opt.md
+++ b/gcc/config/riscv/autovec-opt.md
@@ -1519,6 +1519,7 @@ (define_insn_and_split "*vwsll_zext1_scalar_<mode>"
   {
     if (GET_CODE (operands[2]) == SUBREG)
       operands[2] = SUBREG_REG (operands[2]);
+    operands[2] = gen_lowpart (Pmode, operands[2]);
     insn_code icode = code_for_pred_vwsll_scalar (<MODE>mode);
     riscv_vector::emit_vlmax_insn (icode, riscv_vector::BINARY_OP, operands);
     DONE;
-- 
2.34.1

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

[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 <ewlu@rivosinc.com>
---
 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

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

[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 <rdapp@ventanamicro.com>
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

Re: [PATCH 1/2] RISC-V: Fix vwsll combine on rv32 targets

[Robin Dapp]

Hi Edwin,

this is OK but did you check if we can get rid of the subreg
condition now that we have gen_lowpart?

Regards
 Robin

Re: [PATCH 1/2] RISC-V: Fix vwsll combine on rv32 targets

[Edwin Lu]

Hi Robin,

I did a test run without the subreg condition and it also appears to 
work when running on rv32gcv and rv64gcv newlib. Would it be better to 
remove the subreg?

Edwin

On 6/12/2024 12:42 AM, Robin Dapp wrote:
> Hi Edwin,
>
> this is OK but did you check if we can get rid of the subreg
> condition now that we have gen_lowpart?
>
> Regards
>   Robin

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

[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 <rdapp@ventanamicro.com>
> 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 1/2] RISC-V: Fix vwsll combine on rv32 targets

[Robin Dapp]

> I did a test run without the subreg condition and it also appears to
> work when running on rv32gcv and rv64gcv newlib. Would it be better
> to remove the subreg?

Yep, if it works, i.e. all tests still pass then let's get rid of it.

Regards
 Robin