RISC-V: Add opaque integer modes to fix ICE on DSE[PR111590]
Checks
Commit Message
When doing fortran test with 'V' extension enabled on RISC-V port.
I saw multiple ICE: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111590
The root cause is on DSE:
internal compiler error: in smallest_mode_for_size, at stor-layout.cc:356
0x1918f70 smallest_mode_for_size(poly_int<2u, unsigned long>, mode_class)
../../../../gcc/gcc/stor-layout.cc:356
0x11f75bb smallest_int_mode_for_size(poly_int<2u, unsigned long>)
../../../../gcc/gcc/machmode.h:916
0x3304141 find_shift_sequence
../../../../gcc/gcc/dse.cc:1738
0x3304f1a get_stored_val
../../../../gcc/gcc/dse.cc:1906
0x3305377 replace_read
../../../../gcc/gcc/dse.cc:2010
0x3306226 check_mem_read_rtx
../../../../gcc/gcc/dse.cc:2310
0x330667b check_mem_read_use
../../../../gcc/gcc/dse.cc:2415
After investigations, DSE is trying to do optimization like this following codes:
(insn 86 85 87 9 (set (reg:V4DI 168)
(mem/u/c:V4DI (reg/f:DI 171) [0 S32 A128])) "bug.f90":6:18 discrim 6 1167 {*movv4di}
(expr_list:REG_EQUAL (const_vector:V4DI [
(const_int 4 [0x4])
(const_int 1 [0x1]) repeated x2
(const_int 3 [0x3])
])
(nil)))
(set (mem) (reg:V4DI 168))
Then it ICE on: auto new_mode = smallest_int_mode_for_size (access_size * BITS_PER_UNIT);
The access_size may be 24 or 32. We don't have such integer modes with these size so it ICE.
I saw both aarch64 and ARM has EI/OI/CI/XI opaque modes.
So I add it to walk around ICE on DCE, it works as all ICE are resolved.
CC Richard to review to make sure I am doing the right thing to fix the bug.
Hi, Richard, could you help me with this issue ? Thanks.
gcc/ChangeLog:
* config/riscv/riscv-modes.def (INT_MODE): Add opaque modes
---
gcc/config/riscv/riscv-modes.def | 6 ++++++
1 file changed, 6 insertions(+)
Comments
Juzhe-Zhong <juzhe.zhong@rivai.ai> writes:
> When doing fortran test with 'V' extension enabled on RISC-V port.
> I saw multiple ICE: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111590
>
> The root cause is on DSE:
>
> internal compiler error: in smallest_mode_for_size, at stor-layout.cc:356
> 0x1918f70 smallest_mode_for_size(poly_int<2u, unsigned long>, mode_class)
> ../../../../gcc/gcc/stor-layout.cc:356
> 0x11f75bb smallest_int_mode_for_size(poly_int<2u, unsigned long>)
> ../../../../gcc/gcc/machmode.h:916
> 0x3304141 find_shift_sequence
> ../../../../gcc/gcc/dse.cc:1738
> 0x3304f1a get_stored_val
> ../../../../gcc/gcc/dse.cc:1906
> 0x3305377 replace_read
> ../../../../gcc/gcc/dse.cc:2010
> 0x3306226 check_mem_read_rtx
> ../../../../gcc/gcc/dse.cc:2310
> 0x330667b check_mem_read_use
> ../../../../gcc/gcc/dse.cc:2415
>
> After investigations, DSE is trying to do optimization like this following codes:
>
> (insn 86 85 87 9 (set (reg:V4DI 168)
> (mem/u/c:V4DI (reg/f:DI 171) [0 S32 A128])) "bug.f90":6:18 discrim 6 1167 {*movv4di}
> (expr_list:REG_EQUAL (const_vector:V4DI [
> (const_int 4 [0x4])
> (const_int 1 [0x1]) repeated x2
> (const_int 3 [0x3])
> ])
> (nil)))
>
> (set (mem) (reg:V4DI 168))
>
> Then it ICE on: auto new_mode = smallest_int_mode_for_size (access_size * BITS_PER_UNIT);
>
> The access_size may be 24 or 32. We don't have such integer modes with these size so it ICE.
>
> I saw both aarch64 and ARM has EI/OI/CI/XI opaque modes.
>
> So I add it to walk around ICE on DCE, it works as all ICE are resolved.
>
> CC Richard to review to make sure I am doing the right thing to fix the bug.
>
> Hi, Richard, could you help me with this issue ? Thanks.
>
> gcc/ChangeLog:
>
> * config/riscv/riscv-modes.def (INT_MODE): Add opaque modes
I think it's a bug in dse. The contract is:
/* Find the narrowest integer mode that contains at least SIZE bits.
Such a mode must exist. */
(emphasis on the last line).
The easy fix would be to add:
&& known_le (access_size, GET_MODE_SIZE (MAX_MODE_INT))
The better but more complex fix would be to make dse use native_encode_rtx/
native_decode_rtx (which IIRC didn't exist when the dse code was written).
Thanks,
Richard
>
> ---
> gcc/config/riscv/riscv-modes.def | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/gcc/config/riscv/riscv-modes.def b/gcc/config/riscv/riscv-modes.def
> index e3c6ccb2809..ab86032c914 100644
> --- a/gcc/config/riscv/riscv-modes.def
> +++ b/gcc/config/riscv/riscv-modes.def
> @@ -393,6 +393,12 @@ VLS_MODES (1024); /* V1024QI V512HI V256SI V128DI V512HF V256SF V128DF */
> VLS_MODES (2048); /* V2048QI V1024HI V512SI V256DI V1024HF V512SF V256DF */
> VLS_MODES (4096); /* V4096QI V2048HI V1024SI V512DI V2048HF V1024SF V512DF */
>
> +/* Opaque integer modes 3, 4, 6 or 8 general double registers. */
> +INT_MODE (EI, 24);
> +INT_MODE (OI, 32);
> +INT_MODE (CI, 48);
> +INT_MODE (XI, 64);
> +
> /* TODO: According to RISC-V 'V' ISA spec, the maximun vector length can
> be 65536 for a single vector register which means the vector mode in
> GCC can be maximum = 65536 * 8 bits (LMUL=8).
Thanks Richard.
Is it correct as follows ?
diff --git a/gcc/dse.cc b/gcc/dse.cc
index 8b07be17674..c58d3bf4e1b 100644
--- a/gcc/dse.cc
+++ b/gcc/dse.cc
@@ -1733,7 +1733,7 @@ find_shift_sequence (poly_int64 access_size,
/* If a constant was stored into memory, try to simplify it here,
otherwise the cost of the shift might preclude this optimization
e.g. at -Os, even when no actual shift will be needed. */
- if (store_info->const_rhs)
+ if (store_info->const_rhs && known_le (access_size, GET_MODE_SIZE (MAX_MODE_INT)))
I failed to find native_encode_rtx and native_decode_rtx.
juzhe.zhong@rivai.ai
From: Richard Sandiford
Date: 2023-09-26 21:50
To: Juzhe-Zhong
CC: gcc-patches; kito.cheng; kito.cheng; jeffreyalaw; rdapp.gcc
Subject: Re: [PATCH] RISC-V: Add opaque integer modes to fix ICE on DSE[PR111590]
Juzhe-Zhong <juzhe.zhong@rivai.ai> writes:
> When doing fortran test with 'V' extension enabled on RISC-V port.
> I saw multiple ICE: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111590
>
> The root cause is on DSE:
>
> internal compiler error: in smallest_mode_for_size, at stor-layout.cc:356
> 0x1918f70 smallest_mode_for_size(poly_int<2u, unsigned long>, mode_class)
> ../../../../gcc/gcc/stor-layout.cc:356
> 0x11f75bb smallest_int_mode_for_size(poly_int<2u, unsigned long>)
> ../../../../gcc/gcc/machmode.h:916
> 0x3304141 find_shift_sequence
> ../../../../gcc/gcc/dse.cc:1738
> 0x3304f1a get_stored_val
> ../../../../gcc/gcc/dse.cc:1906
> 0x3305377 replace_read
> ../../../../gcc/gcc/dse.cc:2010
> 0x3306226 check_mem_read_rtx
> ../../../../gcc/gcc/dse.cc:2310
> 0x330667b check_mem_read_use
> ../../../../gcc/gcc/dse.cc:2415
>
> After investigations, DSE is trying to do optimization like this following codes:
>
> (insn 86 85 87 9 (set (reg:V4DI 168)
> (mem/u/c:V4DI (reg/f:DI 171) [0 S32 A128])) "bug.f90":6:18 discrim 6 1167 {*movv4di}
> (expr_list:REG_EQUAL (const_vector:V4DI [
> (const_int 4 [0x4])
> (const_int 1 [0x1]) repeated x2
> (const_int 3 [0x3])
> ])
> (nil)))
>
> (set (mem) (reg:V4DI 168))
>
> Then it ICE on: auto new_mode = smallest_int_mode_for_size (access_size * BITS_PER_UNIT);
>
> The access_size may be 24 or 32. We don't have such integer modes with these size so it ICE.
>
> I saw both aarch64 and ARM has EI/OI/CI/XI opaque modes.
>
> So I add it to walk around ICE on DCE, it works as all ICE are resolved.
>
> CC Richard to review to make sure I am doing the right thing to fix the bug.
>
> Hi, Richard, could you help me with this issue ? Thanks.
>
> gcc/ChangeLog:
>
> * config/riscv/riscv-modes.def (INT_MODE): Add opaque modes
I think it's a bug in dse. The contract is:
/* Find the narrowest integer mode that contains at least SIZE bits.
Such a mode must exist. */
(emphasis on the last line).
The easy fix would be to add:
&& known_le (access_size, GET_MODE_SIZE (MAX_MODE_INT))
The better but more complex fix would be to make dse use native_encode_rtx/
native_decode_rtx (which IIRC didn't exist when the dse code was written).
Thanks,
Richard
>
> ---
> gcc/config/riscv/riscv-modes.def | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/gcc/config/riscv/riscv-modes.def b/gcc/config/riscv/riscv-modes.def
> index e3c6ccb2809..ab86032c914 100644
> --- a/gcc/config/riscv/riscv-modes.def
> +++ b/gcc/config/riscv/riscv-modes.def
> @@ -393,6 +393,12 @@ VLS_MODES (1024); /* V1024QI V512HI V256SI V128DI V512HF V256SF V128DF */
> VLS_MODES (2048); /* V2048QI V1024HI V512SI V256DI V1024HF V512SF V256DF */
> VLS_MODES (4096); /* V4096QI V2048HI V1024SI V512DI V2048HF V1024SF V512DF */
>
> +/* Opaque integer modes 3, 4, 6 or 8 general double registers. */
> +INT_MODE (EI, 24);
> +INT_MODE (OI, 32);
> +INT_MODE (CI, 48);
> +INT_MODE (XI, 64);
> +
> /* TODO: According to RISC-V 'V' ISA spec, the maximun vector length can
> be 65536 for a single vector register which means the vector mode in
> GCC can be maximum = 65536 * 8 bits (LMUL=8).
钟居哲 <juzhe.zhong@rivai.ai> writes:
> Thanks Richard.
>
> Is it correct as follows ?
>
> diff --git a/gcc/dse.cc b/gcc/dse.cc
> index 8b07be17674..c58d3bf4e1b 100644
> --- a/gcc/dse.cc
> +++ b/gcc/dse.cc
> @@ -1733,7 +1733,7 @@ find_shift_sequence (poly_int64 access_size,
> /* If a constant was stored into memory, try to simplify it here,
> otherwise the cost of the shift might preclude this optimization
> e.g. at -Os, even when no actual shift will be needed. */
> - if (store_info->const_rhs)
> + if (store_info->const_rhs && known_le (access_size, GET_MODE_SIZE (MAX_MODE_INT)))
Yes, but the "&& ..." needs to be on a new line because of the 80-character
limit.
> I failed to find native_encode_rtx and native_decode_rtx.
See simplify-rtx.cc.
Richard
@@ -393,6 +393,12 @@ VLS_MODES (1024); /* V1024QI V512HI V256SI V128DI V512HF V256SF V128DF */
VLS_MODES (2048); /* V2048QI V1024HI V512SI V256DI V1024HF V512SF V256DF */
VLS_MODES (4096); /* V4096QI V2048HI V1024SI V512DI V2048HF V1024SF V512DF */
+/* Opaque integer modes 3, 4, 6 or 8 general double registers. */
+INT_MODE (EI, 24);
+INT_MODE (OI, 32);
+INT_MODE (CI, 48);
+INT_MODE (XI, 64);
+
/* TODO: According to RISC-V 'V' ISA spec, the maximun vector length can
be 65536 for a single vector register which means the vector mode in
GCC can be maximum = 65536 * 8 bits (LMUL=8).