Committed, thanks Richard. Pan
-----Original Message----- From: Richard Biener <richard.guent...@gmail.com> Sent: Sunday, March 10, 2024 2:53 PM To: Li, Pan2 <pan2...@intel.com> Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@gmail.com; Wang, Yanzhang <yanzhang.w...@intel.com>; rdapp....@gmail.com; jeffreya...@gmail.com Subject: Re: [PATCH v2] VECT: Fix ICE for vectorizable LD/ST when both len and store are enabled > Am 10.03.2024 um 04:14 schrieb pan2...@intel.com: > > From: Pan Li <pan2...@intel.com> > > This patch would like to fix one ICE in vectorizable_store when both the > loop_masks and loop_lens are enabled. The ICE looks like below when build > with "-march=rv64gcv -O3". > > during GIMPLE pass: vect > test.c: In function ‘d’: > test.c:6:6: internal compiler error: in vectorizable_store, at > tree-vect-stmts.cc:8691 > 6 | void d() { > | ^ > 0x37a6f2f vectorizable_store > .../__RISC-V_BUILD__/../gcc/tree-vect-stmts.cc:8691 > 0x37b861c vect_analyze_stmt(vec_info*, _stmt_vec_info*, bool*, > _slp_tree*, _slp_instance*, vec<stmt_info_for_cost, va_heap, vl_ptr>*) > .../__RISC-V_BUILD__/../gcc/tree-vect-stmts.cc:13242 > 0x1db5dca vect_analyze_loop_operations > .../__RISC-V_BUILD__/../gcc/tree-vect-loop.cc:2208 > 0x1db885b vect_analyze_loop_2 > .../__RISC-V_BUILD__/../gcc/tree-vect-loop.cc:3041 > 0x1dba029 vect_analyze_loop_1 > .../__RISC-V_BUILD__/../gcc/tree-vect-loop.cc:3481 > 0x1dbabad vect_analyze_loop(loop*, vec_info_shared*) > .../__RISC-V_BUILD__/../gcc/tree-vect-loop.cc:3639 > 0x1e389d1 try_vectorize_loop_1 > .../__RISC-V_BUILD__/../gcc/tree-vectorizer.cc:1066 > 0x1e38f3d try_vectorize_loop > .../__RISC-V_BUILD__/../gcc/tree-vectorizer.cc:1182 > 0x1e39230 execute > .../__RISC-V_BUILD__/../gcc/tree-vectorizer.cc:1298 > > There are two ways to reach vectorizer LD/ST, one is the analysis and > the other is transform. We cannot have both the lens and the masks > enabled during transform but it is valid during analysis. Given the > transform doesn't required cost_vec, we can only enable the assert > based on cost_vec is NULL or not. > > Below testsuites are passed for this patch: > * The x86 bootstrap tests. > * The x86 fully regression tests. > * The aarch64 fully regression tests. > * The riscv fully regressison tests. Ok Thanks, Richard > gcc/ChangeLog: > > * tree-vect-stmts.cc (vectorizable_store): Enable the assert > during transform process. > (vectorizable_load): Ditto. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/rvv/base/pr114195-1.c: New test. > > Signed-off-by: Pan Li <pan2...@intel.com> > --- > .../gcc.target/riscv/rvv/base/pr114195-1.c | 15 +++++++++++++++ > gcc/tree-vect-stmts.cc | 18 ++++++++++++++---- > 2 files changed, 29 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr114195-1.c > > diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr114195-1.c > b/gcc/testsuite/gcc.target/riscv/rvv/base/pr114195-1.c > new file mode 100644 > index 00000000000..a67b847112b > --- /dev/null > +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr114195-1.c > @@ -0,0 +1,15 @@ > +/* Test that we do not have ice when compile */ > +/* { dg-do compile } */ > +/* { dg-options "-march=rv64gcv -mabi=lp64d -O3 -ftree-vectorize" } */ > + > +long a, b; > +extern short c[]; > + > +void d() { > + for (int e = 0; e < 35; e = 2) { > + a = ({ a < 0 ? a : 0; }); > + b = ({ b < 0 ? b : 0; }); > + > + c[e] = 0; > + } > +} > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc > index 14a3ffb5f02..e8617439a48 100644 > --- a/gcc/tree-vect-stmts.cc > +++ b/gcc/tree-vect-stmts.cc > @@ -8697,8 +8697,13 @@ vectorizable_store (vec_info *vinfo, > ? &LOOP_VINFO_LENS (loop_vinfo) > : NULL); > > - /* Shouldn't go with length-based approach if fully masked. */ > - gcc_assert (!loop_lens || !loop_masks); > + /* The vect_transform_stmt and vect_analyze_stmt will go here but there > + are some difference here. We cannot enable both the lens and masks > + during transform but it is allowed during analysis. > + Shouldn't go with length-based approach if fully masked. */ > + if (cost_vec == NULL) > + /* The cost_vec is NULL during transfrom. */ > + gcc_assert ((!loop_lens || !loop_masks)); > > /* Targets with store-lane instructions must not require explicit > realignment. vect_supportable_dr_alignment always returns either > @@ -10577,8 +10582,13 @@ vectorizable_load (vec_info *vinfo, > ? &LOOP_VINFO_LENS (loop_vinfo) > : NULL); > > - /* Shouldn't go with length-based approach if fully masked. */ > - gcc_assert (!loop_lens || !loop_masks); > + /* The vect_transform_stmt and vect_analyze_stmt will go here but there > + are some difference here. We cannot enable both the lens and masks > + during transform but it is allowed during analysis. > + Shouldn't go with length-based approach if fully masked. */ > + if (cost_vec == NULL) > + /* The cost_vec is NULL during transfrom. */ > + gcc_assert ((!loop_lens || !loop_masks)); > > /* Targets with store-lane instructions must not require explicit > realignment. vect_supportable_dr_alignment always returns either > -- > 2.34.1 >