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

[PATCH V2 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.

V2: Remove subreg condition and change assert to internal error

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 |  3 +--
 gcc/config/riscv/riscv-v.cc     | 25 +++++++++++++++++++------
 2 files changed, 20 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH V2 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>
---
V2: Remove subreg check
---
 gcc/config/riscv/autovec-opt.md | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gcc/config/riscv/autovec-opt.md b/gcc/config/riscv/autovec-opt.md
index 6a2eabbd854..29916adb62b 100644
--- a/gcc/config/riscv/autovec-opt.md
+++ b/gcc/config/riscv/autovec-opt.md
@@ -1517,8 +1517,7 @@ (define_insn_and_split "*vwsll_zext1_scalar_<mode>"
   "&& 1"
   [(const_int 0)]
   {
-    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 V2 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 and replace it with an internal error for
more descriptive logging

gcc/ChangeLog:

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

Signed-off-by: Edwin Lu <ewlu@rivosinc.com>
Co-authored-by: Robin Dapp <rdapp@ventanamicro.com>
---
V2: change assert to internal error
---
 gcc/config/riscv/riscv-v.cc | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc
index 8911f5783c8..3f8214b74d5 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;
 
@@ -290,11 +291,17 @@ 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.  */
+        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 +353,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.34.1

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

[Robin Dapp]

> diff --git a/gcc/config/riscv/autovec-opt.md b/gcc/config/riscv/autovec-opt.md
> index 6a2eabbd854..29916adb62b 100644
> --- a/gcc/config/riscv/autovec-opt.md
> +++ b/gcc/config/riscv/autovec-opt.md
> @@ -1517,8 +1517,7 @@ (define_insn_and_split "*vwsll_zext1_scalar_<mode>"
>    "&& 1"
>    [(const_int 0)]
>    {
> -    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;

Please also correct it in the other expander (_trunc_).
OK with that change if the testsuite still looks ok :)

Regards
 Robin

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

[Robin Dapp]

OK.

Regards
 Robin