* [PATCH 0/2] Fix ICE with vwsll combine on 32bit targets
@ 2024-06-11 22:25 Edwin Lu
2024-06-11 22:25 ` [PATCH 1/2] RISC-V: Fix vwsll combine on rv32 targets Edwin Lu
2024-06-11 22:25 ` [PATCH 2/2] RISC-V: Move mode assertion out of conditional branch in emit_insn Edwin Lu
0 siblings, 2 replies; 8+ messages in thread
From: Edwin Lu @ 2024-06-11 22:25 UTC (permalink / raw)
To: gcc-patches; +Cc: gnu-toolchain, 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] RISC-V: Fix vwsll combine on rv32 targets
2024-06-11 22:25 [PATCH 0/2] Fix ICE with vwsll combine on 32bit targets Edwin Lu
@ 2024-06-11 22:25 ` Edwin Lu
2024-06-12 7:42 ` Robin Dapp
2024-06-11 22:25 ` [PATCH 2/2] RISC-V: Move mode assertion out of conditional branch in emit_insn Edwin Lu
1 sibling, 1 reply; 8+ messages in thread
From: Edwin Lu @ 2024-06-11 22:25 UTC (permalink / raw)
To: gcc-patches; +Cc: gnu-toolchain, Edwin Lu, Robin Dapp
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] RISC-V: Move mode assertion out of conditional branch in emit_insn
2024-06-11 22:25 [PATCH 0/2] Fix ICE with vwsll combine on 32bit targets Edwin Lu
2024-06-11 22:25 ` [PATCH 1/2] RISC-V: Fix vwsll combine on rv32 targets Edwin Lu
@ 2024-06-11 22:25 ` Edwin Lu
2024-06-12 7:39 ` Robin Dapp
1 sibling, 1 reply; 8+ messages in thread
From: Edwin Lu @ 2024-06-11 22:25 UTC (permalink / raw)
To: gcc-patches; +Cc: gnu-toolchain, 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] RISC-V: Move mode assertion out of conditional branch in emit_insn
2024-06-11 22:25 ` [PATCH 2/2] RISC-V: Move mode assertion out of conditional branch in emit_insn Edwin Lu
@ 2024-06-12 7:39 ` Robin Dapp
2024-06-12 20:25 ` Edwin Lu
0 siblings, 1 reply; 8+ messages in thread
From: Robin Dapp @ 2024-06-12 7:39 UTC (permalink / raw)
To: Edwin Lu, gcc-patches; +Cc: rdapp.gcc, gnu-toolchain
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] RISC-V: Fix vwsll combine on rv32 targets
2024-06-11 22:25 ` [PATCH 1/2] RISC-V: Fix vwsll combine on rv32 targets Edwin Lu
@ 2024-06-12 7:42 ` Robin Dapp
2024-06-12 20:19 ` Edwin Lu
0 siblings, 1 reply; 8+ messages in thread
From: Robin Dapp @ 2024-06-12 7:42 UTC (permalink / raw)
To: Edwin Lu, gcc-patches; +Cc: rdapp.gcc, gnu-toolchain, 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] RISC-V: Fix vwsll combine on rv32 targets
2024-06-12 7:42 ` Robin Dapp
@ 2024-06-12 20:19 ` Edwin Lu
2024-06-13 14:43 ` Robin Dapp
0 siblings, 1 reply; 8+ messages in thread
From: Edwin Lu @ 2024-06-12 20:19 UTC (permalink / raw)
To: Robin Dapp, gcc-patches; +Cc: gnu-toolchain, Robin Dapp
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] RISC-V: Move mode assertion out of conditional branch in emit_insn
2024-06-12 7:39 ` Robin Dapp
@ 2024-06-12 20:25 ` Edwin Lu
0 siblings, 0 replies; 8+ messages in thread
From: Edwin Lu @ 2024-06-12 20:25 UTC (permalink / raw)
To: Robin Dapp, gcc-patches; +Cc: gnu-toolchain
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);
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] RISC-V: Fix vwsll combine on rv32 targets
2024-06-12 20:19 ` Edwin Lu
@ 2024-06-13 14:43 ` Robin Dapp
0 siblings, 0 replies; 8+ messages in thread
From: Robin Dapp @ 2024-06-13 14:43 UTC (permalink / raw)
To: Edwin Lu, gcc-patches; +Cc: rdapp.gcc, gnu-toolchain
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-06-13 14:43 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-06-11 22:25 [PATCH 0/2] Fix ICE with vwsll combine on 32bit targets Edwin Lu
2024-06-11 22:25 ` [PATCH 1/2] RISC-V: Fix vwsll combine on rv32 targets Edwin Lu
2024-06-12 7:42 ` Robin Dapp
2024-06-12 20:19 ` Edwin Lu
2024-06-13 14:43 ` Robin Dapp
2024-06-11 22:25 ` [PATCH 2/2] RISC-V: Move mode assertion out of conditional branch in emit_insn Edwin Lu
2024-06-12 7:39 ` Robin Dapp
2024-06-12 20:25 ` Edwin Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).