Re: [PATCH v2] VECT: Fix ICE for vectorizable LD/ST when both len and store are enabled

[Richard Biener <richard.guenther@gmail.com>]

> Am 10.03.2024 um 11:02 schrieb Li, Pan2 <pan2.li@intel.com>:
> 
> Committed, thanks Richard.

You might want to investigate why you get mask and not Len for a particular stmt.  mixing will cause variable length vectorization to fail.

> Pan
> 
> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: Sunday, March 10, 2024 2:53 PM
> To: Li, Pan2 <pan2.li@intel.com>
> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; rdapp.gcc@gmail.com; jeffreyalaw@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.li@intel.com:
>> 
>> From: Pan Li <pan2.li@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.li@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
>>