[PATCH] Don't ICE on long long shifts in vectorizable_shift

[PATCH] Don't ICE on long long shifts in vectorizable_shift

[Jakub Jelinek]

Hi!

With the patch I'm going to post momentarily which adds vlshrv{4,2}di and
vashlv{4,2}di patterns for -mavx2 vectorizable_shift ICEs, because the
frontends for long_long_var1 << long_long_var2 emit long_long_var1 << (int) long_long_var2
and vectorizable_shift isn't prepared to handle type promotion (or
demotion).  IMHO it would complicate it too much, so this patch just
gives up on vectorizing in that case.

I can work on Monday on pattern recognizer that will change
shifts/rotates where the rhs1 has different size from rhs2 into
a pattern with def_stmt casting the rhs2 to the same type as rhs1
and pattern_stmt that uses the temporary for rhs2, perhaps with extra
optimization if it sees the type being promoted/demoted again to just look
through those.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2011-10-27  Jakub Jelinek  <jakub@redhat.com>

	* tree-vect-stmts.c (vectorizable_shift): Give up if op1 has different
	vector mode from vectype's mode.

--- gcc/tree-vect-stmts.c.jj	2011-10-27 08:42:51.000000000 +0200
+++ gcc/tree-vect-stmts.c	2011-10-27 17:24:15.000000000 +0200
@@ -2318,6 +2318,7 @@ vectorizable_shift (gimple stmt, gimple_
   int nunits_in;
   int nunits_out;
   tree vectype_out;
+  tree op1_vectype;
   int ncopies;
   int j, i;
   VEC (tree, heap) *vec_oprnds0 = NULL, *vec_oprnds1 = NULL;
@@ -2387,7 +2388,8 @@ vectorizable_shift (gimple stmt, gimple_
     return false;
 
   op1 = gimple_assign_rhs2 (stmt);
-  if (!vect_is_simple_use (op1, loop_vinfo, bb_vinfo, &def_stmt, &def, &dt[1]))
+  if (!vect_is_simple_use_1 (op1, loop_vinfo, bb_vinfo, &def_stmt, &def,
+			     &dt[1], &op1_vectype))
     {
       if (vect_print_dump_info (REPORT_DETAILS))
         fprintf (vect_dump, "use not simple.");
@@ -2444,6 +2446,13 @@ vectorizable_shift (gimple stmt, gimple_
       optab = optab_for_tree_code (code, vectype, optab_vector);
       if (vect_print_dump_info (REPORT_DETAILS))
         fprintf (vect_dump, "vector/vector shift/rotate found.");
+      if (TYPE_MODE (op1_vectype) != TYPE_MODE (vectype))
+	{
+	  if (vect_print_dump_info (REPORT_DETAILS))
+	    fprintf (vect_dump, "unusable type for last operand in"
+				" vector/vector shift/rotate.");
+	  return false;
+	}
     }
   /* See if the machine has a vector shifted by scalar insn and if not
      then see if it has a vector shifted by vector insn.  */

	Jakub

Re: [PATCH] Don't ICE on long long shifts in vectorizable_shift

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3497 bytes --]

On Thu, 27 Oct 2011, Jakub Jelinek wrote:

> Hi!
> 
> With the patch I'm going to post momentarily which adds vlshrv{4,2}di and
> vashlv{4,2}di patterns for -mavx2 vectorizable_shift ICEs, because the
> frontends for long_long_var1 << long_long_var2 emit long_long_var1 << (int) long_long_var2
> and vectorizable_shift isn't prepared to handle type promotion (or
> demotion).  IMHO it would complicate it too much, so this patch just
> gives up on vectorizing in that case.
> 
> I can work on Monday on pattern recognizer that will change
> shifts/rotates where the rhs1 has different size from rhs2 into
> a pattern with def_stmt casting the rhs2 to the same type as rhs1
> and pattern_stmt that uses the temporary for rhs2, perhaps with extra
> optimization if it sees the type being promoted/demoted again to just look
> through those.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Hm, but you are testing vector modes in the path that is supposed to
handle shifts by a scalar.  That looks odd.  Also it should be easy
to promote/demote the scalar value to the proper type, so why not
do that?  Like how we do

              /* Unlike the other binary operators, shifts/rotates have
                 the rhs being int, instead of the same type as the lhs,
                 so make sure the scalar is the right type if we are
                 dealing with vectors of short/char.  */
              if (dt[1] == vect_constant_def)
                op1 = fold_convert (TREE_TYPE (vectype), op1);

just do that for all kind of defs (and deal with the fact that you
may need a gimplified stmt for the vector shift amount generation).

?

Thanks,
Richard.


> 2011-10-27  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* tree-vect-stmts.c (vectorizable_shift): Give up if op1 has different
> 	vector mode from vectype's mode.
> 
> --- gcc/tree-vect-stmts.c.jj	2011-10-27 08:42:51.000000000 +0200
> +++ gcc/tree-vect-stmts.c	2011-10-27 17:24:15.000000000 +0200
> @@ -2318,6 +2318,7 @@ vectorizable_shift (gimple stmt, gimple_
>    int nunits_in;
>    int nunits_out;
>    tree vectype_out;
> +  tree op1_vectype;
>    int ncopies;
>    int j, i;
>    VEC (tree, heap) *vec_oprnds0 = NULL, *vec_oprnds1 = NULL;
> @@ -2387,7 +2388,8 @@ vectorizable_shift (gimple stmt, gimple_
>      return false;
>  
>    op1 = gimple_assign_rhs2 (stmt);
> -  if (!vect_is_simple_use (op1, loop_vinfo, bb_vinfo, &def_stmt, &def, &dt[1]))
> +  if (!vect_is_simple_use_1 (op1, loop_vinfo, bb_vinfo, &def_stmt, &def,
> +			     &dt[1], &op1_vectype))
>      {
>        if (vect_print_dump_info (REPORT_DETAILS))
>          fprintf (vect_dump, "use not simple.");
> @@ -2444,6 +2446,13 @@ vectorizable_shift (gimple stmt, gimple_
>        optab = optab_for_tree_code (code, vectype, optab_vector);
>        if (vect_print_dump_info (REPORT_DETAILS))
>          fprintf (vect_dump, "vector/vector shift/rotate found.");
> +      if (TYPE_MODE (op1_vectype) != TYPE_MODE (vectype))
> +	{
> +	  if (vect_print_dump_info (REPORT_DETAILS))
> +	    fprintf (vect_dump, "unusable type for last operand in"
> +				" vector/vector shift/rotate.");
> +	  return false;
> +	}
>      }
>    /* See if the machine has a vector shifted by scalar insn and if not
>       then see if it has a vector shifted by vector insn.  */
> 
> 	Jakub
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer

Re: [PATCH] Don't ICE on long long shifts in vectorizable_shift

[Jakub Jelinek]

On Fri, Oct 28, 2011 at 10:22:15AM +0200, Richard Guenther wrote:
> Hm, but you are testing vector modes in the path that is supposed to
> handle shifts by a scalar.  That looks odd.  Also it should be easy

No, I'm testing it in the path that is supposed to handle shifts by a
vector.  That block starts with:
  /* Vector shifted by vector.  */
  if (!scalar_shift_arg)
    {

Which means I'd have to duplicate there big parts of
vectorizable_type_promotion and vectorizable_type_demotion
and handle all these widening resp. narrowing cases.

I know you don't like tree-vect-pattern.c too much, but IMHO just
changing the shifts in there to have rhs2 type matching rhs1 type
would be far easier and more maintainable.  Especially when it can handle
additionally what one of the testcases does - long long shift
with long long shift count, which should be naturally vectorized without
any promotion/demotion, but the FEs insert there a cast to (int) which would
result in the promotion/demotion.

	Jakub

Re: [PATCH] Don't ICE on long long shifts in vectorizable_shift

[Richard Guenther]

On Fri, 28 Oct 2011, Jakub Jelinek wrote:

> On Fri, Oct 28, 2011 at 10:22:15AM +0200, Richard Guenther wrote:
> > Hm, but you are testing vector modes in the path that is supposed to
> > handle shifts by a scalar.  That looks odd.  Also it should be easy
> 
> No, I'm testing it in the path that is supposed to handle shifts by a
> vector.  That block starts with:
>   /* Vector shifted by vector.  */
>   if (!scalar_shift_arg)
>     {

Oh, I looked close and thought you are touching the else { part ...

> Which means I'd have to duplicate there big parts of
> vectorizable_type_promotion and vectorizable_type_demotion
> and handle all these widening resp. narrowing cases.

Yeah, agreed (though it should be always possible to just
truncate/extend the shift count).

> I know you don't like tree-vect-pattern.c too much, but IMHO just
> changing the shifts in there to have rhs2 type matching rhs1 type
> would be far easier and more maintainable.  Especially when it can handle
> additionally what one of the testcases does - long long shift
> with long long shift count, which should be naturally vectorized without
> any promotion/demotion, but the FEs insert there a cast to (int) which would
> result in the promotion/demotion.

... but we could as well forward-prop this (also for scalar code)
(well, truncations only for SHIFT_COUNT_TRUNCATED targets).

The patch is ok meanwhile.

Thanks,
Richard.