Thanks Richard.
> so the obvious fix would be to add
>
> if (!VECTOR_MODE_P (loop_vinfo->vector_mode))
> return false;
I also think of it, but it is too "easy" and then dropped.
> Ah, it needs -march=rv64imd_xsfvcp.
It can also be reproduced by " -march=rv64imd_zve32x -mrvv-vector-bits=zvl",
sorry forgot to mention this.
> The error is probably that vect_verify_loop_lens does not do anything
> to ensure the checks are done on a relevant mode. With the suggested
> added check above this then becomes a missed optimization rather
> than an ICE. But it might fall apart if there's not one load/store len mode
> to consider?
I see, it may fall apart I am afraid, consider RVVM1DImode when rv64gc_zve32x,
the riscv_vector_mode_supported_any_target_p will always return true and we may
have RVVM1DImode here but zve32x cannot support DI as element size.
I will try to reproduce this after this ICE fix.
Pan
-----Original Message-----
From: Richard Biener <richard.guent...@gmail.com>
Sent: Tuesday, February 18, 2025 5:36 PM
To: Li, Pan2 <pan2...@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@gmail.com;
jeffreya...@gmail.com; rdapp....@gmail.com
Subject: Re: [PATCH v1] Vect: Fix ICE when get DImode from
get_related_vectype_for_scalar_type [PR116351]
On Tue, Feb 18, 2025 at 10:12 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Tue, Feb 18, 2025 at 9:40 AM Li, Pan2 <pan2...@intel.com> wrote:
> >
> > Hi Richard,
> >
> > After some more investigation, the sample code never hit one vectorizable_*
> > routines which may check the loop_vinfo->vector_mode,
> > and then the loop_vinfo->vector_mode == DImode will hit the
> > vect_verify_loop_lens and trigger the assert VECTOR_MODE_P, detail
> > flow as below.
> >
> > vect_analyze_loop_2
> > |- vect_pattern_recog // Hit over-widening pattern and set
> > loop_vinfo->vector_mode to DImode
> > |- ...
> > |- vect_analyze_loop_operations
> > |- (gdb) p stmt_info->def_type
> > |- $1 = vect_reduction_def
> > |- (gdb) p stmt_info->slp_type
> > |- $2 = pure_slp
> > |- vectorizable_lc_phi // Not Hit
> > |- vectorizable_induction // Not Hit
> > |- vectorizable_reduction // Not Hit
> > |- vectorizable_recurr // Not Hit
> > |- vectorizable_live_operation // Not Hit
> > |- vect_analyze_stmt
> > |- (gdb) p stmt_info->relevant
> > |- $3 = vect_unused_in_scope
> > |- (gdb) p stmt_info->live
> > |- $4 = false
> > |- (gdb) p pattern_stmt_info
> > |- $5 = (stmt_vec_info) 0x0
> > |- return opt_result::success ();
> > OR
> > |- PURE_SLP_STMT (stmt_info) && !node then dump "handled only by SLP
> > analysis\n"
> > |- Early return opt_result::success ();
> > |- vectorizable_load/store/call_convert/... // Not Hit
> > |- LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P && !LOOP_VINFO_MASKS
> > (loop_vinfo).is_empty ()
> > |- vect_verify_loop_lens (loop_vinfo)
> > |- assert (VECTOR_MODE_P (loop_vinfo->vector_mode); // Hit assert
> > result in ICE
> >
> > I am a little hesitant by two options here.
> >
> > 1. shall we add some condition and dump log here to make the
> > vect_analyze_loop_2 failure when loop_vinfo->vector_mode is not supported
> > vector mode by target.
> > 2. it should not be LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P here? Then we need
> > to find out where set the partial vector to true.
> >
> > Is there any suggestion here?
>
> static bool
> vect_verify_loop_lens (loop_vec_info loop_vinfo)
> {
> if (LOOP_VINFO_LENS (loop_vinfo).is_empty ())
> return false;
>
> machine_mode len_load_mode, len_store_mode;
> if (!get_len_load_store_mode (loop_vinfo->vector_mode, true)
> .exists (&len_load_mode))
> return false;
>
> so the obvious fix would be to add
>
> if (!VECTOR_MODE_P (loop_vinfo->vector_mode))
> return false;
>
> here? But then I wonder how we got to a DImode vector_mode and record
> a loop len
> in the first place. I could imagine we first end up with DImode but
> other stmts using
> a vector mode and we record a len for those. But then the above
> get_len_load_store_mode
> on ->vector_mode seems to assume that all modes we need a len for are
> "compatible" with ->vector_mode so I assume recording a LEN would check that.
>
> I can't reproduce the ICE with a cross on trunk btw.
Ah, it needs -march=rv64imd_xsfvcp. So we indeed call vect_record_loop_len
with
(gdb) p debug_tree (vectype)
<vector_type 0x7ffff71bd1f8
type <integer_type 0x7ffff7017690 short int public HI
size <integer_cst 0x7ffff7041558 constant 16>
unit-size <integer_cst 0x7ffff7041570 constant 2>
align:16 warn_if_not_align:0 symtab:0 alias-set 2
canonical-type 0x7ffff7017690 precision:16 min <integer_cst
0x7ffff7041510 -32768> max <integer_cst 0x7ffff7041528 32767>
pointer_to_this <pointer_type 0x7ffff71b4348>>
RVVM2HI
(gdb) p loop_vinfo->vector_mode
$2 = E_DImode
from vectorizable_operation and ->vector_mode is set via
vect_recog_over_widening_pattern which commits to a DImode
vector type ->vector_mode prematurely.
The error is probably that vect_verify_loop_lens does not do anything
to ensure the checks are done on a relevant mode. With the suggested
added check above this then becomes a missed optimization rather
than an ICE. But it might fall apart if there's not one load/store len mode
to consider?
>
> Richard.
>
> >
> > Pan
> >
> > -----Original Message-----
> > From: Li, Pan2
> > Sent: Monday, February 17, 2025 6:08 PM
> > To: Richard Biener <richard.guent...@gmail.com>
> > Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@gmail.com;
> > jeffreya...@gmail.com; rdapp....@gmail.com
> > Subject: RE: [PATCH v1] Vect: Fix ICE when get DImode from
> > get_related_vectype_for_scalar_type [PR116351]
> >
> > > But that's wrong - read the comment before the code. We do support
> > > integer mode
> > > "generic" vectorization just fine. Iff there's anything to plug then
> > > it's how we end
> > > up thinking there's with_len support for DImode vectors.
> >
> > I see, then we need another place to fix this, let me have a try.
> >
> > Pan
> >
> > -----Original Message-----
> > From: Richard Biener <richard.guent...@gmail.com>
> > Sent: Monday, February 17, 2025 6:02 PM
> > To: Li, Pan2 <pan2...@intel.com>
> > Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@gmail.com;
> > jeffreya...@gmail.com; rdapp....@gmail.com
> > Subject: Re: [PATCH v1] Vect: Fix ICE when get DImode from
> > get_related_vectype_for_scalar_type [PR116351]
> >
> > On Mon, Feb 17, 2025 at 10:38 AM <pan2...@intel.com> wrote:
> > >
> > > From: Pan Li <pan2...@intel.com>
> > >
> > > This patch would like to fix the ICE similar as below, assump we have
> > > sample code:
> > >
> > > 1 │ int a, b, c;
> > > 2 │ short d, e, f;
> > > 3 │ long g (long h) { return h; }
> > > 4 │
> > > 5 │ void i () {
> > > 6 │ for (; b; ++b) {
> > > 7 │ f = 5 >> a ? d : d << a;
> > > 8 │ e &= c | g(f);
> > > 9 │ }
> > > 10 │ }
> > >
> > > It will ice when compile with -O3 -march=rv64gc_zve64f
> > > -mrvv-vector-bits=zvl
> > >
> > > during GIMPLE pass: vect
> > > pr116351-1.c: In function ‘i’:
> > > pr116351-1.c:8:6: internal compiler error: in get_len_load_store_mode,
> > > at optabs-tree.cc:655
> > > 8 | void i () {
> > > | ^
> > > 0x44d6b9d internal_error(char const*, ...)
> > > /home/pli/gcc/111/riscv-gnu-toolchain/gcc/__RISC-V_BUILD__/../gcc/diagnostic-global-context.cc:517
> > > 0x44a26a6 fancy_abort(char const*, int, char const*)
> > >
> > > /home/pli/gcc/111/riscv-gnu-toolchain/gcc/__RISC-V_BUILD__/../gcc/diagnostic.cc:1722
> > > 0x19e4309 get_len_load_store_mode(machine_mode, bool, internal_fn*,
> > > vec<int, va_heap, vl_ptr>*)
> > >
> > > /home/pli/gcc/111/riscv-gnu-toolchain/gcc/__RISC-V_BUILD__/../gcc/optabs-tree.cc:655
> > > 0x1fada40 vect_verify_loop_lens
> > >
> > > /home/pli/gcc/111/riscv-gnu-toolchain/gcc/__RISC-V_BUILD__/../gcc/tree-vect-loop.cc:1566
> > > 0x1fb2b07 vect_analyze_loop_2
> > > /home/pli/gcc/111/riscv-gnu-toolchain/gcc/__RISC-V_BUILD__/../gcc/tree-vect-loop.cc:3037
> > > 0x1fb4302 vect_analyze_loop_1
> > >
> > > /home/pli/gcc/111/riscv-gnu-toolchain/gcc/__RISC-V_BUILD__/../gcc/tree-vect-loop.cc:3478
> > > 0x1fb4e9a vect_analyze_loop(loop*, gimple*, vec_info_shared*)
> > >
> > > /home/pli/gcc/111/riscv-gnu-toolchain/gcc/__RISC-V_BUILD__/../gcc/tree-vect-loop.cc:3638
> > > 0x203c2dc try_vectorize_loop_1
> > >
> > > /home/pli/gcc/111/riscv-gnu-toolchain/gcc/__RISC-V_BUILD__/../gcc/tree-vectorizer.cc:1095
> > > 0x203c839 try_vectorize_loop
> > >
> > > /home/pli/gcc/111/riscv-gnu-toolchain/gcc/__RISC-V_BUILD__/../gcc/tree-vectorizer.cc:1212
> > > 0x203cb2c execute
> > >
> > > The zve32x cannot have 64 elen, and then the
> > > get_related_vectype_for_scalar_type
> > > will get DImode as vector_mode in loop_info. After that the underlying
> > > vect_analyze_xx will assert the mode is VECTOR and then ICE at the assert.
> > >
> > > The fix contains 2 part, aka let the get_related_vectype_for_scalar_type
> > > return NULL_TREE if mode_for_vector is not VECTOR mode in the middle-end,
> > > and then mark the innermode of RVV is DImode is not support when the
> > > TARGET_VECTOR_ELEN_64 is false.
> > >
> > > The below test suites are passed for this patch.
> > > * The rv64gcv fully regression test.
> > > * The x86 bootstrap test.
> > > * The x86 fully regression test.
> > >
> > > PR target/116351
> > >
> > > gcc/ChangeLog:
> > >
> > > * config/riscv/riscv.cc
> > > (riscv_vector_mode_supported_any_target_p): Mark
> > > innnermode of RVV is DImode unsupported when zve32*.
> > > * tree-vect-stmts.cc (get_related_vectype_for_scalar_type): Return
> > > the NULL_TREE if mode_for_vector is not a VECTOR mode.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > * gcc.target/riscv/rvv/base/pr116351-1.c: New test.
> > > * gcc.target/riscv/rvv/base/pr116351-2.c: New test.
> > > * gcc.target/riscv/rvv/base/pr116351.h: New test.
> > >
> > > Signed-off-by: Pan Li <pan2...@intel.com>
> > > ---
> > > gcc/config/riscv/riscv.cc | 6 +++++-
> > > .../gcc.target/riscv/rvv/base/pr116351-1.c | 5 +++++
> > > .../gcc.target/riscv/rvv/base/pr116351-2.c | 5 +++++
> > > .../gcc.target/riscv/rvv/base/pr116351.h | 18 ++++++++++++++++++
> > > gcc/tree-vect-stmts.cc | 9 +++++++--
> > > 5 files changed, 40 insertions(+), 3 deletions(-)
> > > create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr116351-1.c
> > > create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr116351-2.c
> > > create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr116351.h
> > >
> > > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> > > index 9bf7713139f..89b534ac88f 100644
> > > --- a/gcc/config/riscv/riscv.cc
> > > +++ b/gcc/config/riscv/riscv.cc
> > > @@ -12613,10 +12613,14 @@ extract_base_offset_in_addr (rtx mem, rtx
> > > *base, rtx *offset)
> > > /* Implements target hook vector_mode_supported_any_target_p. */
> > >
> > > static bool
> > > -riscv_vector_mode_supported_any_target_p (machine_mode)
> > > +riscv_vector_mode_supported_any_target_p (machine_mode mode)
> > > {
> > > if (TARGET_XTHEADVECTOR)
> > > return false;
> > > +
> > > + if (GET_MODE_INNER (mode) == DImode && !TARGET_VECTOR_ELEN_64)
> > > + return false;
> > > +
> > > return true;
> > > }
> > >
> > > diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr116351-1.c
> > > b/gcc/testsuite/gcc.target/riscv/rvv/base/pr116351-1.c
> > > new file mode 100644
> > > index 00000000000..f58fedfeaf1
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr116351-1.c
> > > @@ -0,0 +1,5 @@
> > > +/* Test that we do not have ice when compile */
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-march=rv64gc_zve32x -mabi=lp64d -O3 -ftree-vectorize"
> > > } */
> > > +
> > > +#include "pr116351.h"
> > > diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr116351-2.c
> > > b/gcc/testsuite/gcc.target/riscv/rvv/base/pr116351-2.c
> > > new file mode 100644
> > > index 00000000000..e1f46b745e2
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr116351-2.c
> > > @@ -0,0 +1,5 @@
> > > +/* Test that we do not have ice when compile */
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-march=rv64gc_zve32f -mabi=lp64d -O3 -ftree-vectorize"
> > > } */
> > > +
> > > +#include "pr116351.h"
> > > diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr116351.h
> > > b/gcc/testsuite/gcc.target/riscv/rvv/base/pr116351.h
> > > new file mode 100644
> > > index 00000000000..25bd0ec65e0
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr116351.h
> > > @@ -0,0 +1,18 @@
> > > +#ifndef PR116351_H
> > > +#define PR116351_H
> > > +
> > > +#define T long
> > > +
> > > +int a, b, c;
> > > +short d, e, f;
> > > +
> > > +T g (T h) { return h; }
> > > +
> > > +void i () {
> > > + for (; b; ++b) {
> > > + f = 5 >> a ? d : d << a;
> > > + e &= c | g(f);
> > > + }
> > > +}
> > > +
> > > +#endif
> > > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
> > > index 6bbb16beff2..a4a959185da 100644
> > > --- a/gcc/tree-vect-stmts.cc
> > > +++ b/gcc/tree-vect-stmts.cc
> > > @@ -14228,8 +14228,13 @@ get_related_vectype_for_scalar_type
> > > (machine_mode prevailing_mode,
> > >
> > > Note that nunits == 1 is allowed in order to support single
> > > element vector types. */
> > > - if (!multiple_p (GET_MODE_SIZE (simd_mode), nbytes, &nunits)
> > > - || !mode_for_vector (inner_mode, nunits).exists
> > > (&simd_mode))
> > > + if (!multiple_p (GET_MODE_SIZE (simd_mode), nbytes, &nunits))
> > > + return NULL_TREE;
> > > +
> > > + bool exist_p = mode_for_vector (inner_mode,
> > > + nunits).exists (&simd_mode);
> > > +
> > > + if (!exist_p || (exist_p && !VECTOR_MODE_P (simd_mode)))
> >
> > But that's wrong - read the comment before the code. We do support integer
> > mode
> > "generic" vectorization just fine. Iff there's anything to plug then
> > it's how we end
> > up thinking there's with_len support for DImode vectors.
> >
> > Richard.
> >
> > > return NULL_TREE;
> > > }
> > > }
> > > --
> > > 2.43.0
> > >
