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