[PATCH] RISC-V: Add opaque integer modes to fix ICE on DSE[PR111590]

[PATCH] RISC-V: Add opaque integer modes to fix ICE on DSE[PR111590]

[Juzhe-Zhong]

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(+)

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).
-- 
2.36.3

Re: [PATCH] RISC-V: Add opaque integer modes to fix ICE on DSE[PR111590]

[Richard Sandiford]

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).

Re: Re: [PATCH] RISC-V: Add opaque integer modes to fix ICE on DSE[PR111590]

[钟居哲]

[-- Attachment #1: Type: text/plain, Size: 4239 bytes --]

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).
 

Re: [PATCH] RISC-V: Add opaque integer modes to fix ICE on DSE[PR111590]

[Richard Sandiford]

钟居哲 <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