diff mbox series

[6/19] middle-end: Don't enter piecewise expansion if VF is not constant.

Message ID ZJw5OtpY61bsCZBR@arm.com
State Not Applicable
Headers
Series Support early break/return auto-vectorization |

Checks

Context Check Description
snail/gcc-patch-check fail Git am fail log

Commit Message

Tamar Christina June 28, 2023, 1:44 p.m. UTC 
  Hi All,

expand_vector_piecewise does not support VLA expansion as it has a hard assert
on the type not being VLA.

Instead of just failing to expand and so the call marked unsupported we ICE.
This adjust it so we don't and can gracefully handle the expansion in support
checks.

Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.

Ok for master?

Thanks,
Tamar

gcc/ChangeLog:

	* tree-vect-generic.cc (expand_vector_comparison): Skip piecewise if not
	constant.

--- inline copy of patch -- 
diff --git a/gcc/tree-vect-generic.cc b/gcc/tree-vect-generic.cc
index df04a0db68da3222f43dd938f8e7adb186cd93c9..da1fd2f40d82a9fa301e6ed0b2f4c3c222d58a8d 100644




--
diff --git a/gcc/tree-vect-generic.cc b/gcc/tree-vect-generic.cc
index df04a0db68da3222f43dd938f8e7adb186cd93c9..da1fd2f40d82a9fa301e6ed0b2f4c3c222d58a8d 100644
--- a/gcc/tree-vect-generic.cc
+++ b/gcc/tree-vect-generic.cc
@@ -481,7 +481,7 @@ expand_vector_comparison (gimple_stmt_iterator *gsi, tree type, tree op0,
 	    }
 	  t = gimplify_build1 (gsi, VIEW_CONVERT_EXPR, type, t);
 	}
-      else
+      else if (TYPE_VECTOR_SUBPARTS (type).is_constant ())
 	t = expand_vector_piecewise (gsi, do_compare, type,
 				     TREE_TYPE (TREE_TYPE (op0)), op0, op1,
 				     code, false);

Comments

Richard Biener July 4, 2023, 12:10 p.m. UTC | #1 
On Wed, 28 Jun 2023, Tamar Christina wrote:

> Hi All,
> 
> expand_vector_piecewise does not support VLA expansion as it has a hard assert
> on the type not being VLA.
> 
> Instead of just failing to expand and so the call marked unsupported we ICE.
> This adjust it so we don't and can gracefully handle the expansion in support
> checks.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> 
> Ok for master?

Hmm, do we support _any_ VLA "generic" vectors?  That is, why do
we get here at all?  Doesn't that mean the vectorizer creates
code that vector lowering thinks is not supported by the target?

In any case I'd expect expand_vector_operations_1 at

  if (compute_type == NULL_TREE)
    compute_type = get_compute_type (code, op, type);
  if (compute_type == type)
    return;

 <----  here

  new_rhs = expand_vector_operation (gsi, type, compute_type, stmt, code,
                                     dce_ssa_names);

to be able to assert that compute_type (or even type) isn't VLA?

So, why do we arrive here?

Richard.


> Thanks,
> Tamar
> 
> gcc/ChangeLog:
> 
> 	* tree-vect-generic.cc (expand_vector_comparison): Skip piecewise if not
> 	constant.
> 
> --- inline copy of patch -- 
> diff --git a/gcc/tree-vect-generic.cc b/gcc/tree-vect-generic.cc
> index df04a0db68da3222f43dd938f8e7adb186cd93c9..da1fd2f40d82a9fa301e6ed0b2f4c3c222d58a8d 100644
> --- a/gcc/tree-vect-generic.cc
> +++ b/gcc/tree-vect-generic.cc
> @@ -481,7 +481,7 @@ expand_vector_comparison (gimple_stmt_iterator *gsi, tree type, tree op0,
>  	    }
>  	  t = gimplify_build1 (gsi, VIEW_CONVERT_EXPR, type, t);
>  	}
> -      else
> +      else if (TYPE_VECTOR_SUBPARTS (type).is_constant ())
>  	t = expand_vector_piecewise (gsi, do_compare, type,
>  				     TREE_TYPE (TREE_TYPE (op0)), op0, op1,
>  				     code, false);
> 
> 
> 
> 
>
Tamar Christina July 6, 2023, 10:37 a.m. UTC | #2 
> On Wed, 28 Jun 2023, Tamar Christina wrote:
> 
> > Hi All,
> >
> > expand_vector_piecewise does not support VLA expansion as it has a
> > hard assert on the type not being VLA.
> >
> > Instead of just failing to expand and so the call marked unsupported we ICE.
> > This adjust it so we don't and can gracefully handle the expansion in
> > support checks.
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> >
> > Ok for master?
> 
> Hmm, do we support _any_ VLA "generic" vectors?  That is, why do we get
> here at all?  Doesn't that mean the vectorizer creates code that vector lowering
> thinks is not supported by the target?
> 
> In any case I'd expect expand_vector_operations_1 at
> 
>   if (compute_type == NULL_TREE)
>     compute_type = get_compute_type (code, op, type);
>   if (compute_type == type)
>     return;
> 
>  <----  here
> 
>   new_rhs = expand_vector_operation (gsi, type, compute_type, stmt, code,
>                                      dce_ssa_names);
> 
> to be able to assert that compute_type (or even type) isn't VLA?
> 
> So, why do we arrive here?
> 

I think we used to arrive here because the patch last year didn't properly check the cmp,
I don't his it with this new patch so I'll drop it.  I thought it was an actual bug hence why I
submitted the patch 😊

Thanks,
Tamar
> Richard.
> 
> 
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> > 	* tree-vect-generic.cc (expand_vector_comparison): Skip piecewise if
> not
> > 	constant.
> >
> > --- inline copy of patch --
> > diff --git a/gcc/tree-vect-generic.cc b/gcc/tree-vect-generic.cc index
> >
> df04a0db68da3222f43dd938f8e7adb186cd93c9..da1fd2f40d82a9fa301e6
> ed0b2f4
> > c3c222d58a8d 100644
> > --- a/gcc/tree-vect-generic.cc
> > +++ b/gcc/tree-vect-generic.cc
> > @@ -481,7 +481,7 @@ expand_vector_comparison (gimple_stmt_iterator
> *gsi, tree type, tree op0,
> >  	    }
> >  	  t = gimplify_build1 (gsi, VIEW_CONVERT_EXPR, type, t);
> >  	}
> > -      else
> > +      else if (TYPE_VECTOR_SUBPARTS (type).is_constant ())
> >  	t = expand_vector_piecewise (gsi, do_compare, type,
> >  				     TREE_TYPE (TREE_TYPE (op0)), op0, op1,
> >  				     code, false);
> >
> >
> >
> >
> >
> 
> --
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461
> Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald,
> Boudien Moerman; HRB 36809 (AG Nuernberg)
Richard Biener July 6, 2023, 10:51 a.m. UTC | #3 
On Thu, 6 Jul 2023, Tamar Christina wrote:

> > On Wed, 28 Jun 2023, Tamar Christina wrote:
> > 
> > > Hi All,
> > >
> > > expand_vector_piecewise does not support VLA expansion as it has a
> > > hard assert on the type not being VLA.
> > >
> > > Instead of just failing to expand and so the call marked unsupported we ICE.
> > > This adjust it so we don't and can gracefully handle the expansion in
> > > support checks.
> > >
> > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> > >
> > > Ok for master?
> > 
> > Hmm, do we support _any_ VLA "generic" vectors?  That is, why do we get
> > here at all?  Doesn't that mean the vectorizer creates code that vector lowering
> > thinks is not supported by the target?
> > 
> > In any case I'd expect expand_vector_operations_1 at
> > 
> >   if (compute_type == NULL_TREE)
> >     compute_type = get_compute_type (code, op, type);
> >   if (compute_type == type)
> >     return;
> > 
> >  <----  here
> > 
> >   new_rhs = expand_vector_operation (gsi, type, compute_type, stmt, code,
> >                                      dce_ssa_names);
> > 
> > to be able to assert that compute_type (or even type) isn't VLA?
> > 
> > So, why do we arrive here?
> > 
> 
> I think we used to arrive here because the patch last year didn't properly check the cmp,
> I don't his it with this new patch so I'll drop it.  I thought it was an actual bug hence why I
> submitted the patch ?

If it's a genuine bug then the fix at least looks wrong ;)

Anyway, dropping is fine with me of course.

Richard.
diff mbox series

Patch

--- a/gcc/tree-vect-generic.cc
+++ b/gcc/tree-vect-generic.cc
@@ -481,7 +481,7 @@  expand_vector_comparison (gimple_stmt_iterator *gsi, tree type, tree op0,
 	    }
 	  t = gimplify_build1 (gsi, VIEW_CONVERT_EXPR, type, t);
 	}
-      else
+      else if (TYPE_VECTOR_SUBPARTS (type).is_constant ())
 	t = expand_vector_piecewise (gsi, do_compare, type,
 				     TREE_TYPE (TREE_TYPE (op0)), op0, op1,
 				     code, false);