[PATCH] fix useless unsharing of SLP tree

[PATCH] fix useless unsharing of SLP tree

[Richard Biener]

This avoids unsharing the SLP tree when optimizing load permutations
for reductions but there is no actual permute taking place.

Bootstrapped and tested on x86_64-unknown-linux-gnu, pushed.

2020-09-09  Richard Biener  <rguenther@suse.de>

	* tree-vect-slp.c (vect_attempt_slp_rearrange_stmts): Do
	nothing when the permutation doesn't permute.
---
 gcc/tree-vect-slp.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
index 03b11058bd5..15d57890b6f 100644
--- a/gcc/tree-vect-slp.c
+++ b/gcc/tree-vect-slp.c
@@ -1905,11 +1905,14 @@ vect_attempt_slp_rearrange_stmts (slp_instance slp_instn)
     }
 
   /* Check that the loads in the first sequence are different and there
-     are no gaps between them.  */
+     are no gaps between them and that there is an actual permutation.  */
+  bool any_permute = false;
   auto_sbitmap load_index (group_size);
   bitmap_clear (load_index);
   FOR_EACH_VEC_ELT (node->load_permutation, i, lidx)
     {
+      if (lidx != i)
+	any_permute = true;
       if (lidx >= group_size)
 	return false;
       if (bitmap_bit_p (load_index, lidx))
@@ -1917,6 +1920,8 @@ vect_attempt_slp_rearrange_stmts (slp_instance slp_instn)
 
       bitmap_set_bit (load_index, lidx);
     }
+  if (!any_permute)
+    return false;
   for (i = 0; i < group_size; i++)
     if (!bitmap_bit_p (load_index, i))
       return false;
-- 
2.26.2

Re: [PATCH] fix useless unsharing of SLP tree

[Richard Sandiford]

Richard Biener <rguenther@suse.de> writes:
> This avoids unsharing the SLP tree when optimizing load permutations
> for reductions but there is no actual permute taking place.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu, pushed.
>
> 2020-09-09  Richard Biener  <rguenther@suse.de>
>
> 	* tree-vect-slp.c (vect_attempt_slp_rearrange_stmts): Do
> 	nothing when the permutation doesn't permute.

Drive-by comment, sorry for not looking in more detail first, but:
is there a difference in semantics between an identity load_permutation
and a null load_permutation?  Seems like it would be good to have
a canonical form.

Thanks,
Richard

> ---
>  gcc/tree-vect-slp.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
> index 03b11058bd5..15d57890b6f 100644
> --- a/gcc/tree-vect-slp.c
> +++ b/gcc/tree-vect-slp.c
> @@ -1905,11 +1905,14 @@ vect_attempt_slp_rearrange_stmts (slp_instance slp_instn)
>      }
>  
>    /* Check that the loads in the first sequence are different and there
> -     are no gaps between them.  */
> +     are no gaps between them and that there is an actual permutation.  */
> +  bool any_permute = false;
>    auto_sbitmap load_index (group_size);
>    bitmap_clear (load_index);
>    FOR_EACH_VEC_ELT (node->load_permutation, i, lidx)
>      {
> +      if (lidx != i)
> +	any_permute = true;
>        if (lidx >= group_size)
>  	return false;
>        if (bitmap_bit_p (load_index, lidx))
> @@ -1917,6 +1920,8 @@ vect_attempt_slp_rearrange_stmts (slp_instance slp_instn)
>  
>        bitmap_set_bit (load_index, lidx);
>      }
> +  if (!any_permute)
> +    return false;
>    for (i = 0; i < group_size; i++)
>      if (!bitmap_bit_p (load_index, i))
>        return false;

Re: [PATCH] fix useless unsharing of SLP tree

[Richard Biener]

On Thu, 10 Sep 2020, Richard Sandiford wrote:

> Richard Biener <rguenther@suse.de> writes:
> > This avoids unsharing the SLP tree when optimizing load permutations
> > for reductions but there is no actual permute taking place.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu, pushed.
> >
> > 2020-09-09  Richard Biener  <rguenther@suse.de>
> >
> > 	* tree-vect-slp.c (vect_attempt_slp_rearrange_stmts): Do
> > 	nothing when the permutation doesn't permute.
> 
> Drive-by comment, sorry for not looking in more detail first, but:
> is there a difference in semantics between an identity load_permutation
> and a null load_permutation?  Seems like it would be good to have
> a canonical form.

Well, yes - an identity load_permutation will become non-identity
if there's a GROUP_GAP and we end up with a VF > 1.  We're simply
postponing figuring this out until we have VF fixed (and
vect_attempt_slp_rearrange_stmts is quite legacy now and at an
odd place - sth I hope to address).  So with GROUP_GAP == 1
and an identity permute the permute should be NULL.

I've run into this during development of other stuff but decided
to push this fix since it might as well happen for the SLP
reduction case.  It's probably a regression from when I pushed
permute validity check to vectorizable_load.

Richard.

> Thanks,
> Richard
> 
> > ---
> >  gcc/tree-vect-slp.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
> > index 03b11058bd5..15d57890b6f 100644
> > --- a/gcc/tree-vect-slp.c
> > +++ b/gcc/tree-vect-slp.c
> > @@ -1905,11 +1905,14 @@ vect_attempt_slp_rearrange_stmts (slp_instance slp_instn)
> >      }
> >  
> >    /* Check that the loads in the first sequence are different and there
> > -     are no gaps between them.  */
> > +     are no gaps between them and that there is an actual permutation.  */
> > +  bool any_permute = false;
> >    auto_sbitmap load_index (group_size);
> >    bitmap_clear (load_index);
> >    FOR_EACH_VEC_ELT (node->load_permutation, i, lidx)
> >      {
> > +      if (lidx != i)
> > +	any_permute = true;
> >        if (lidx >= group_size)
> >  	return false;
> >        if (bitmap_bit_p (load_index, lidx))
> > @@ -1917,6 +1920,8 @@ vect_attempt_slp_rearrange_stmts (slp_instance slp_instn)
> >  
> >        bitmap_set_bit (load_index, lidx);
> >      }
> > +  if (!any_permute)
> > +    return false;
> >    for (i = 0; i < group_size; i++)
> >      if (!bitmap_bit_p (load_index, i))
> >        return false;
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)

Re: [PATCH] fix useless unsharing of SLP tree

[Richard Sandiford]

Richard Biener <rguenther@suse.de> writes:
> On Thu, 10 Sep 2020, Richard Sandiford wrote:
>
>> Richard Biener <rguenther@suse.de> writes:
>> > This avoids unsharing the SLP tree when optimizing load permutations
>> > for reductions but there is no actual permute taking place.
>> >
>> > Bootstrapped and tested on x86_64-unknown-linux-gnu, pushed.
>> >
>> > 2020-09-09  Richard Biener  <rguenther@suse.de>
>> >
>> > 	* tree-vect-slp.c (vect_attempt_slp_rearrange_stmts): Do
>> > 	nothing when the permutation doesn't permute.
>> 
>> Drive-by comment, sorry for not looking in more detail first, but:
>> is there a difference in semantics between an identity load_permutation
>> and a null load_permutation?  Seems like it would be good to have
>> a canonical form.
>
> Well, yes - an identity load_permutation will become non-identity
> if there's a GROUP_GAP and we end up with a VF > 1.  We're simply
> postponing figuring this out until we have VF fixed (and
> vect_attempt_slp_rearrange_stmts is quite legacy now and at an
> odd place - sth I hope to address).  So with GROUP_GAP == 1
> and an identity permute the permute should be NULL.
>
> I've run into this during development of other stuff but decided
> to push this fix since it might as well happen for the SLP
> reduction case.  It's probably a regression from when I pushed
> permute validity check to vectorizable_load.

Ah, OK, thanks.

Richard