[PATCH] Fix mixed input kind permute optimization

[PATCH] Fix mixed input kind permute optimization

[Richard Biener]

When change_vec_perm_layout runs into a permute combining two
nodes where one is invariant and one internal the partition of
one input can be -1 but the other might not be.  The following
supports this case by simply ignoring inputs with input partiton -1.

I'm not sure this is correct but it avoids ICEing when accessing
that partitions layout for gcc.target/i386/pr98928.c with the
change to avoid splitting store dataref groups during SLP discovery.

Bootstrap and regtest running on x86_64-unknown-linux-gnu (ontop of
the SLP series).  The change can't break anything that's already
broken but I'm not sure this does the right thing - the testcase
has an uniform constant.  I'll try to come up with a better runtime
testcase tomorrow.  Hints as to where to correctly fix such case
appreciated.

	* tree-vect-slp.cc (change_vec_perm_layout): Ignore an
	input partition of -1.
---
 gcc/tree-vect-slp.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc
index 873748b0a72..f6ec1a81c96 100644
--- a/gcc/tree-vect-slp.cc
+++ b/gcc/tree-vect-slp.cc
@@ -4828,6 +4828,8 @@ change_vec_perm_layout (slp_tree node, lane_permutation_t &perm,
 	{
 	  slp_tree in_node = SLP_TREE_CHILDREN (node)[entry.first];
 	  unsigned int in_partition_i = m_vertices[in_node->vertex].partition;
+	  if (in_partition_i == -1u)
+	    continue;
 	  this_in_layout_i = m_partitions[in_partition_i].layout;
 	}
       if (this_in_layout_i > 0)
-- 
2.35.3

Re: [PATCH] Fix mixed input kind permute optimization

[Richard Sandiford]

Richard Biener <rguenther@suse.de> writes:
> When change_vec_perm_layout runs into a permute combining two
> nodes where one is invariant and one internal the partition of
> one input can be -1 but the other might not be.  The following
> supports this case by simply ignoring inputs with input partiton -1.
>
> I'm not sure this is correct but it avoids ICEing when accessing
> that partitions layout for gcc.target/i386/pr98928.c with the
> change to avoid splitting store dataref groups during SLP discovery.
>
> Bootstrap and regtest running on x86_64-unknown-linux-gnu (ontop of
> the SLP series).  The change can't break anything that's already
> broken but I'm not sure this does the right thing - the testcase
> has an uniform constant.  I'll try to come up with a better runtime
> testcase tomorrow.  Hints as to where to correctly fix such case
> appreciated.

Famous last words, but yeah, it looks correct to me.  I think the
routine in principle should have a free choice of which layout to
choose for invariants (as long as it's consistent for all queries
about the same node).  So it should just be a question of whether
keeping the original layout is more likely to give a valid
permutation, or whether going with out_layout_i would be better.
I don't have a strong intuition either way.

Thanks,
Richard

>
> 	* tree-vect-slp.cc (change_vec_perm_layout): Ignore an
> 	input partition of -1.
> ---
>  gcc/tree-vect-slp.cc | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc
> index 873748b0a72..f6ec1a81c96 100644
> --- a/gcc/tree-vect-slp.cc
> +++ b/gcc/tree-vect-slp.cc
> @@ -4828,6 +4828,8 @@ change_vec_perm_layout (slp_tree node, lane_permutation_t &perm,
>  	{
>  	  slp_tree in_node = SLP_TREE_CHILDREN (node)[entry.first];
>  	  unsigned int in_partition_i = m_vertices[in_node->vertex].partition;
> +	  if (in_partition_i == -1u)
> +	    continue;
>  	  this_in_layout_i = m_partitions[in_partition_i].layout;
>  	}
>        if (this_in_layout_i > 0)

Re: [PATCH] Fix mixed input kind permute optimization

[Richard Sandiford]

Richard Sandiford <richard.sandiford@arm.com> writes:
> Richard Biener <rguenther@suse.de> writes:
>> When change_vec_perm_layout runs into a permute combining two
>> nodes where one is invariant and one internal the partition of
>> one input can be -1 but the other might not be.  The following
>> supports this case by simply ignoring inputs with input partiton -1.
>>
>> I'm not sure this is correct but it avoids ICEing when accessing
>> that partitions layout for gcc.target/i386/pr98928.c with the
>> change to avoid splitting store dataref groups during SLP discovery.
>>
>> Bootstrap and regtest running on x86_64-unknown-linux-gnu (ontop of
>> the SLP series).  The change can't break anything that's already
>> broken but I'm not sure this does the right thing - the testcase
>> has an uniform constant.  I'll try to come up with a better runtime
>> testcase tomorrow.  Hints as to where to correctly fix such case
>> appreciated.
>
> Famous last words, but yeah, it looks correct to me.  I think the
> routine in principle should have a free choice of which layout to
> choose for invariants (as long as it's consistent for all queries
> about the same node).  So it should just be a question of whether
> keeping the original layout is more likely to give a valid
> permutation, or whether going with out_layout_i would be better.
> I don't have a strong intuition either way.

BTW, I should have said that using a different layout from 0
would require compensating code in the materialize function.
So this is definitely the simplest and most direct fix.

Thanks,
Richard

Re: [PATCH] Fix mixed input kind permute optimization

[Richard Biener]

On Wed, 22 May 2024, Richard Sandiford wrote:

> Richard Sandiford <richard.sandiford@arm.com> writes:
> > Richard Biener <rguenther@suse.de> writes:
> >> When change_vec_perm_layout runs into a permute combining two
> >> nodes where one is invariant and one internal the partition of
> >> one input can be -1 but the other might not be.  The following
> >> supports this case by simply ignoring inputs with input partiton -1.
> >>
> >> I'm not sure this is correct but it avoids ICEing when accessing
> >> that partitions layout for gcc.target/i386/pr98928.c with the
> >> change to avoid splitting store dataref groups during SLP discovery.
> >>
> >> Bootstrap and regtest running on x86_64-unknown-linux-gnu (ontop of
> >> the SLP series).  The change can't break anything that's already
> >> broken but I'm not sure this does the right thing - the testcase
> >> has an uniform constant.  I'll try to come up with a better runtime
> >> testcase tomorrow.  Hints as to where to correctly fix such case
> >> appreciated.
> >
> > Famous last words, but yeah, it looks correct to me.  I think the
> > routine in principle should have a free choice of which layout to
> > choose for invariants (as long as it's consistent for all queries
> > about the same node).  So it should just be a question of whether
> > keeping the original layout is more likely to give a valid
> > permutation, or whether going with out_layout_i would be better.
> > I don't have a strong intuition either way.
> 
> BTW, I should have said that using a different layout from 0
> would require compensating code in the materialize function.
> So this is definitely the simplest and most direct fix.

Yeah, I guess we can improve on that later.  I'm going to push the
change after lunch together with the other two fixes - the ARM CI
discovered its share of testsuite fallout for the actual change
I'm going to look at.

Richard.