[PATCH v2][RFC] vect: Verify that GET_MODE_NUNITS is greater than one for vect_grouped_store_supported

[PATCH v2][RFC] vect: Verify that GET_MODE_NUNITS is greater than one for vect_grouped_store_supported

[Kevin Lee]

This patch is a proper fix to the previous patch 
https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614463.html 
vect_grouped_store_supported checks if the count is a power of 2, but
doesn't check the size of the GET_MODE_NUNITS.
This should handle the riscv case where the mode is VNx1DI since the
nelt would be {1, 1}. 
It was tested on RISCV and x86_64-linux-gnu. Would this be correct 
for the vectors with size smaller than 2?

---
 gcc/tree-vect-data-refs.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gcc/tree-vect-data-refs.cc b/gcc/tree-vect-data-refs.cc
index 8daf7bd7dd3..04ad12f7d04 100644
--- a/gcc/tree-vect-data-refs.cc
+++ b/gcc/tree-vect-data-refs.cc
@@ -5399,6 +5399,8 @@ vect_grouped_store_supported (tree vectype, unsigned HOST_WIDE_INT count)
 	  poly_uint64 nelt = GET_MODE_NUNITS (mode);
 
 	  /* The encoding has 2 interleaved stepped patterns.  */
+    if(!nelt.is_constant() && maybe_lt(nelt, (unsigned int) 2))
+      return false;
 	  vec_perm_builder sel (nelt, 2, 3);
 	  sel.quick_grow (6);
 	  for (i = 0; i < 3; i++)
-- 
2.25.1

Re: [PATCH v2][RFC] vect: Verify that GET_MODE_NUNITS is greater than one for vect_grouped_store_supported

[Richard Biener]

On Mon, Mar 27, 2023 at 6:02 PM Kevin Lee <kevinl@rivosinc.com> wrote:
>
> This patch is a proper fix to the previous patch
> https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614463.html
> vect_grouped_store_supported checks if the count is a power of 2, but
> doesn't check the size of the GET_MODE_NUNITS.
> This should handle the riscv case where the mode is VNx1DI since the
> nelt would be {1, 1}.
> It was tested on RISCV and x86_64-linux-gnu. Would this be correct
> for the vectors with size smaller than 2?
>
> ---
>  gcc/tree-vect-data-refs.cc | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/gcc/tree-vect-data-refs.cc b/gcc/tree-vect-data-refs.cc
> index 8daf7bd7dd3..04ad12f7d04 100644
> --- a/gcc/tree-vect-data-refs.cc
> +++ b/gcc/tree-vect-data-refs.cc
> @@ -5399,6 +5399,8 @@ vect_grouped_store_supported (tree vectype, unsigned HOST_WIDE_INT count)
>           poly_uint64 nelt = GET_MODE_NUNITS (mode);
>
>           /* The encoding has 2 interleaved stepped patterns.  */
> +    if(!nelt.is_constant() && maybe_lt(nelt, (unsigned int) 2))
> +      return false;

Indentation is off (or your MUA is broken).  I think the nelt.is_constant ()
check is superfluous but with constant nelt we'd never end up with a
grouped store.

Note the calls are guarded with

         && ! known_eq (TYPE_VECTOR_SUBPARTS (vectype), 1U)

maybe the better fix is to change those to ! maybe_eq?

Richard should know best here.

Richard.

>           vec_perm_builder sel (nelt, 2, 3);
>           sel.quick_grow (6);
>           for (i = 0; i < 3; i++)
> --
> 2.25.1
>

Re: [PATCH v2][RFC] vect: Verify that GET_MODE_NUNITS is greater than one for vect_grouped_store_supported

[Richard Sandiford]

Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On Mon, Mar 27, 2023 at 6:02 PM Kevin Lee <kevinl@rivosinc.com> wrote:
>>
>> This patch is a proper fix to the previous patch
>> https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614463.html
>> vect_grouped_store_supported checks if the count is a power of 2, but
>> doesn't check the size of the GET_MODE_NUNITS.
>> This should handle the riscv case where the mode is VNx1DI since the
>> nelt would be {1, 1}.
>> It was tested on RISCV and x86_64-linux-gnu. Would this be correct
>> for the vectors with size smaller than 2?
>>
>> ---
>>  gcc/tree-vect-data-refs.cc | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/gcc/tree-vect-data-refs.cc b/gcc/tree-vect-data-refs.cc
>> index 8daf7bd7dd3..04ad12f7d04 100644
>> --- a/gcc/tree-vect-data-refs.cc
>> +++ b/gcc/tree-vect-data-refs.cc
>> @@ -5399,6 +5399,8 @@ vect_grouped_store_supported (tree vectype, unsigned HOST_WIDE_INT count)
>>           poly_uint64 nelt = GET_MODE_NUNITS (mode);
>>
>>           /* The encoding has 2 interleaved stepped patterns.  */
>> +    if(!nelt.is_constant() && maybe_lt(nelt, (unsigned int) 2))
>> +      return false;
>
> Indentation is off (or your MUA is broken).  I think the nelt.is_constant ()
> check is superfluous but with constant nelt we'd never end up with a
> grouped store.
>
> Note the calls are guarded with
>
>          && ! known_eq (TYPE_VECTOR_SUBPARTS (vectype), 1U)
>
> maybe the better fix is to change those to ! maybe_eq?

I think the point of those checks is that a grouped store of N 1-element
vectors is equivalent to a store of N scalars.  Nothing needs to happen
internally within the vectors.

For a grouped store of VNx1 vectors, some permutation would be needed.
But it's difficult to generate code for that case, because the minimum
size reduces to two scalars while larger sizes need normal interleaves.

But I think the better check for location above is:

   if (!multiple_p (nelt, 2))
     return false;

which then guards the assert in the later exact_div (nelt, 2).

Thanks,
Richard

Re: [PATCH v2][RFC] vect: Verify that GET_MODE_NUNITS is greater than one for vect_grouped_store_supported

[Kevin Lee]

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

Thank you for the feedback Richard and Richard.

> Note the calls are guarded with
>
>       && ! known_eq (TYPE_VECTOR_SUBPARTS (vectype), 1U)

Yes, I believe nelt.is_constant() wouldn't be necessary. I didn't realize
the call was guarded by this condition.

> But I think the better check for location above is:
>
>    if (!multiple_p (nelt, 2))
>     return false;
>
> which then guards the assert in the later exact_div (nelt, 2).

I believe this check is better than using maybe_lt because it properly
guards exact_div(nelt, 2) and vec_perm_builder sel(nelt, 2, 3).
I'll modify the patch accordingly, build, test and submit the patch. Thank
you!!

Sincerely,

[PATCH v3] vect: Verify that GET_MODE_UNITS is greater than one for vect_grouped_store_supported

[Kevin Lee]

This patch properly guards gcc_assert (multiple_p (m_full_nelts, 
m_npatterns)) in vec_perm_indices indices (sel, 2, nelt) for VNx1 vectors.

Based on the feedback from Richard Biener and Richard Sandiford,
multiple_p has been used instead of maybe_lt to compare nelt with the
minimum size 2.

Bootstrap and testing done on x86_64-pc-linux-gnu. Would this be ok for trunk?

Patch V1: https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614463.html
Patch V2: https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614700.html
 
Kevin Lee <kevinl@rivosinc.com>
gcc/ChangeLog:

	* tree-vect-data-refs.cc (vect_grouped_store_supported): Add new
condition
---
 gcc/tree-vect-data-refs.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gcc/tree-vect-data-refs.cc b/gcc/tree-vect-data-refs.cc
index 8daf7bd7dd3..df393ba723d 100644
--- a/gcc/tree-vect-data-refs.cc
+++ b/gcc/tree-vect-data-refs.cc
@@ -5399,6 +5399,8 @@ vect_grouped_store_supported (tree vectype, unsigned HOST_WIDE_INT count)
 	  poly_uint64 nelt = GET_MODE_NUNITS (mode);
 
 	  /* The encoding has 2 interleaved stepped patterns.  */
+    if(!multiple_p (nelt, 2))
+      return false;
 	  vec_perm_builder sel (nelt, 2, 3);
 	  sel.quick_grow (6);
 	  for (i = 0; i < 3; i++)
-- 
2.25.1

Re: [PATCH v3] vect: Verify that GET_MODE_UNITS is greater than one for vect_grouped_store_supported

[Richard Biener]

On Mon, Apr 17, 2023 at 6:40 PM Kevin Lee <kevinl@rivosinc.com> wrote:
>
> This patch properly guards gcc_assert (multiple_p (m_full_nelts,
> m_npatterns)) in vec_perm_indices indices (sel, 2, nelt) for VNx1 vectors.
>
> Based on the feedback from Richard Biener and Richard Sandiford,
> multiple_p has been used instead of maybe_lt to compare nelt with the
> minimum size 2.
>
> Bootstrap and testing done on x86_64-pc-linux-gnu. Would this be ok for trunk?

Yes, this is OK.

Thanks,
Richard.

> Patch V1: https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614463.html
> Patch V2: https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614700.html
>
> Kevin Lee <kevinl@rivosinc.com>
> gcc/ChangeLog:
>
>         * tree-vect-data-refs.cc (vect_grouped_store_supported): Add new
> condition
> ---
>  gcc/tree-vect-data-refs.cc | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/gcc/tree-vect-data-refs.cc b/gcc/tree-vect-data-refs.cc
> index 8daf7bd7dd3..df393ba723d 100644
> --- a/gcc/tree-vect-data-refs.cc
> +++ b/gcc/tree-vect-data-refs.cc
> @@ -5399,6 +5399,8 @@ vect_grouped_store_supported (tree vectype, unsigned HOST_WIDE_INT count)
>           poly_uint64 nelt = GET_MODE_NUNITS (mode);
>
>           /* The encoding has 2 interleaved stepped patterns.  */
> +    if(!multiple_p (nelt, 2))
> +      return false;
>           vec_perm_builder sel (nelt, 2, 3);
>           sel.quick_grow (6);
>           for (i = 0; i < 3; i++)
> --
> 2.25.1
>

Re: [PATCH v3] vect: Verify that GET_MODE_UNITS is greater than one for vect_grouped_store_supported

[Jeff Law]

On 4/17/23 10:38, Kevin Lee wrote:
> This patch properly guards gcc_assert (multiple_p (m_full_nelts,
> m_npatterns)) in vec_perm_indices indices (sel, 2, nelt) for VNx1 vectors.
> 
> Based on the feedback from Richard Biener and Richard Sandiford,
> multiple_p has been used instead of maybe_lt to compare nelt with the
> minimum size 2.
> 
> Bootstrap and testing done on x86_64-pc-linux-gnu. Would this be ok for trunk?
> 
> Patch V1: https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614463.html
> Patch V2: https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614700.html
>   
> Kevin Lee <kevinl@rivosinc.com>
> gcc/ChangeLog:
> 
> 	* tree-vect-data-refs.cc (vect_grouped_store_supported): Add new
> condition.
I fixed up the indentation and pushed this to the trunk.

jeff