diff mbox

Fix up vectorization of multiplication with bool cast to integer (PR tree-optimization/69820)

Message ID alpine.LSU.2.11.1602160952120.1392@t29.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Feb. 16, 2016, 8:58 a.m. UTC 
On Mon, 15 Feb 2016, Jakub Jelinek wrote:

> Hi!
> 
> The following testcase is miscompiled, because we first
> create a pattern stmt for _5 = (int) _3; where _3 is bool,
> but then recognize the following multiply as widening multiply, ignore
> there the previous pattern stmt and thus instead of expanding the
> cast as cond ? 1 : 0 we actually end up expanding it as cond (i.e.
> cond ? -1 : 0).  In the widen mult pattern recognizer it perhaps would be
> possible to handle that case, unless both arguments are cast from
> bool, but there are lots of other places which call type_conversion_p and
> most of them would need to either give up in those cases or add special
> handling for it.  So, it seems like the easiest fix at least for GCC6 is
> to punt in type_conversion_p on casts from bool/unsigned :1 (or should
> I instead check STMT_VINFO_IN_PATTERN_P on the cast stmt and give up
> if true?).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Hmm, most places calling type_conversion_p already check for the
pattern case and only accept a very specific or fail.

Only widen_mul/sum don't do that.

I believe type_conversion_p should simply look at the detected pattern,
thus

does that fix it?

Thanks,
Richard.


> 2016-02-15  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/69820
> 	* tree-vect-patterns.c (type_conversion_p): Return false if
> 	*orig_type is unsigned single precision or boolean.
> 	(vect_recog_dot_prod_pattern, vect_recog_widen_mult_pattern):
> 	Formatting fix.
> 
> 	* gcc.dg/vect/pr69820.c: New test.
> 
> --- gcc/tree-vect-patterns.c.jj	2016-01-12 14:14:56.000000000 +0100
> +++ gcc/tree-vect-patterns.c	2016-02-15 18:52:52.695249972 +0100
> @@ -184,6 +184,13 @@ type_conversion_p (tree name, gimple *us
>        || ((TYPE_UNSIGNED (type) != TYPE_UNSIGNED (*orig_type)) && check_sign))
>      return false;
>  
> +  /* Conversion from bool or unsigned single bit precision bitfields
> +     should have been recognized by vect_recog_bool_pattern, callers
> +     of this function are generally unprepared to handle those.  */
> +  if ((TYPE_PRECISION (*orig_type) == 1 && TYPE_UNSIGNED (*orig_type))
> +      || TREE_CODE (*orig_type) == BOOLEAN_TYPE)
> +    return false;
> +
>    if (TYPE_PRECISION (type) >= (TYPE_PRECISION (*orig_type) * 2))
>      *promotion = true;
>    else
> @@ -334,8 +341,8 @@ vect_recog_dot_prod_pattern (vec<gimple
>        stmt = last_stmt;
>  
>        if (type_conversion_p (oprnd0, stmt, true, &half_type, &def_stmt,
> -                               &promotion)
> -         && promotion)
> +			     &promotion)
> +	  && promotion)
>          {
>            stmt = def_stmt;
>            oprnd0 = gimple_assign_rhs1 (stmt);
> @@ -395,13 +402,13 @@ vect_recog_dot_prod_pattern (vec<gimple
>            || !types_compatible_p (TREE_TYPE (oprnd1), prod_type))
>          return NULL;
>        if (!type_conversion_p (oprnd0, stmt, true, &half_type0, &def_stmt,
> -                                &promotion)
> -          || !promotion)
> +			      &promotion)
> +	  || !promotion)
>          return NULL;
>        oprnd00 = gimple_assign_rhs1 (def_stmt);
>        if (!type_conversion_p (oprnd1, stmt, true, &half_type1, &def_stmt,
> -                                &promotion)
> -          || !promotion)
> +			      &promotion)
> +	  || !promotion)
>          return NULL;
>        oprnd01 = gimple_assign_rhs1 (def_stmt);
>        if (!types_compatible_p (half_type0, half_type1))
> @@ -891,10 +898,10 @@ vect_recog_widen_mult_pattern (vec<gimpl
>  	  oprnd = &oprnd1;
>  	}
>  
> -        tree old_oprnd = gimple_assign_rhs1 (def_stmt);
> -	tree new_oprnd = make_ssa_name (half_type0);
> -	new_stmt = gimple_build_assign (new_oprnd, NOP_EXPR, old_oprnd);
> -        *oprnd = new_oprnd;
> +      tree old_oprnd = gimple_assign_rhs1 (def_stmt);
> +      tree new_oprnd = make_ssa_name (half_type0);
> +      new_stmt = gimple_build_assign (new_oprnd, NOP_EXPR, old_oprnd);
> +      *oprnd = new_oprnd;
>      }
>  
>    /* Handle unsigned case.  Look for
> --- gcc/testsuite/gcc.dg/vect/pr69820.c.jj	2016-02-15 18:54:15.333122283 +0100
> +++ gcc/testsuite/gcc.dg/vect/pr69820.c	2016-02-15 18:55:45.529891445 +0100
> @@ -0,0 +1,35 @@
> +/* PR tree-optimization/69820 */
> +
> +#include "tree-vect.h"
> +
> +unsigned int a[100];
> +long long int b[100];
> +unsigned short c[100];
> +
> +__attribute__((noinline, noclone)) void
> +foo (void)
> +{
> +  int i;
> +  for (i = 0; i < 100; ++i)
> +    b[i] = a[i] * (c[i] * (_Bool) c[i]);
> +}
> +
> +int
> +main ()
> +{
> +  int i;
> +  if (__SIZEOF_INT__ * __CHAR_BIT__ != 32)
> +    return 0;
> +  check_vect ();
> +  for (i = 0; i < 100; ++i)
> +    {
> +      a[i] = 3489456818U;
> +      b[i] = 0x1eadbeefbeefdeadLL;
> +      c[i] = 38364;
> +    }
> +  foo ();
> +  for (i = 0; i < 100; ++i)
> +    if (b[i] != 0xed446af8U)
> +      __builtin_abort ();
> +  return 0;
> +}
> 
> 	Jakub
> 
>

Comments

Jakub Jelinek Feb. 16, 2016, 9:26 a.m. UTC | #1 
On Tue, Feb 16, 2016 at 09:58:37AM +0100, Richard Biener wrote:
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> Hmm, most places calling type_conversion_p already check for the
> pattern case and only accept a very specific or fail.
> 
> Only widen_mul/sum don't do that.
> 
> I believe type_conversion_p should simply look at the detected pattern,
> thus
> 
> Index: tree-vect-patterns.c
> ===================================================================
> --- tree-vect-patterns.c        (revision 233418)
> +++ tree-vect-patterns.c        (working copy)
> @@ -171,6 +171,13 @@ type_conversion_p (tree name, gimple *us
>    if (!*def_stmt)
>      return false;
>  
> +  if (dt == vect_internal_def)
> +    {
> +      stmt_vec_info def_vinfo = vinfo_for_stmt (*def_stmt);
> +      if (STMT_VINFO_IN_PATTERN_P (def_vinfo))
> +       *def_stmt = STMT_VINFO_RELATED_STMT (def_vinfo);
> +    }
> +
>    if (!is_gimple_assign (*def_stmt))
>      return false;
>  
> does that fix it?

This doesn't work.

pr69820.c: In function ‘foo’:
pr69820.c:6:1: internal compiler error: in vect_get_vec_def_for_operand, at tree-vect-stmts.c:1424
 foo (void)
 ^~~
0x106f8c6 vect_get_vec_def_for_operand(tree_node*, gimple*, tree_node*)
	../../gcc/tree-vect-stmts.c:1424
0x1078daf vectorizable_conversion
	../../gcc/tree-vect-stmts.c:4087
0x1085da5 vect_transform_stmt(gimple*, gimple_stmt_iterator*, bool*, _slp_tree*, _slp_instance*)
	../../gcc/tree-vect-stmts.c:8191
...

I think the vectorizer is not prepared to see one pattern seq referencing
SSA_NAME set by a different pattern seq.

	Jakub
Richard Biener Feb. 16, 2016, 10:06 a.m. UTC | #2 
On Tue, 16 Feb 2016, Jakub Jelinek wrote:

> On Tue, Feb 16, 2016 at 09:58:37AM +0100, Richard Biener wrote:
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > Hmm, most places calling type_conversion_p already check for the
> > pattern case and only accept a very specific or fail.
> > 
> > Only widen_mul/sum don't do that.
> > 
> > I believe type_conversion_p should simply look at the detected pattern,
> > thus
> > 
> > Index: tree-vect-patterns.c
> > ===================================================================
> > --- tree-vect-patterns.c        (revision 233418)
> > +++ tree-vect-patterns.c        (working copy)
> > @@ -171,6 +171,13 @@ type_conversion_p (tree name, gimple *us
> >    if (!*def_stmt)
> >      return false;
> >  
> > +  if (dt == vect_internal_def)
> > +    {
> > +      stmt_vec_info def_vinfo = vinfo_for_stmt (*def_stmt);
> > +      if (STMT_VINFO_IN_PATTERN_P (def_vinfo))
> > +       *def_stmt = STMT_VINFO_RELATED_STMT (def_vinfo);
> > +    }
> > +
> >    if (!is_gimple_assign (*def_stmt))
> >      return false;
> >  
> > does that fix it?
> 
> This doesn't work.
> 
> pr69820.c: In function ‘foo’:
> pr69820.c:6:1: internal compiler error: in vect_get_vec_def_for_operand, at tree-vect-stmts.c:1424
>  foo (void)
>  ^~~
> 0x106f8c6 vect_get_vec_def_for_operand(tree_node*, gimple*, tree_node*)
> 	../../gcc/tree-vect-stmts.c:1424
> 0x1078daf vectorizable_conversion
> 	../../gcc/tree-vect-stmts.c:4087
> 0x1085da5 vect_transform_stmt(gimple*, gimple_stmt_iterator*, bool*, _slp_tree*, _slp_instance*)
> 	../../gcc/tree-vect-stmts.c:8191
> ...
> 
> I think the vectorizer is not prepared to see one pattern seq referencing
> SSA_NAME set by a different pattern seq.

Hmm, I think it works in general, but I suspect that the pattern has
to use the original def but for out analysis we have to look at the
pattern.

So another fix would be to simply fail if there was a pattern detected.

Richard.
diff mbox

Patch

Index: tree-vect-patterns.c
===================================================================
--- tree-vect-patterns.c        (revision 233418)
+++ tree-vect-patterns.c        (working copy)
@@ -171,6 +171,13 @@  type_conversion_p (tree name, gimple *us
   if (!*def_stmt)
     return false;
 
+  if (dt == vect_internal_def)
+    {
+      stmt_vec_info def_vinfo = vinfo_for_stmt (*def_stmt);
+      if (STMT_VINFO_IN_PATTERN_P (def_vinfo))
+       *def_stmt = STMT_VINFO_RELATED_STMT (def_vinfo);
+    }
+
   if (!is_gimple_assign (*def_stmt))
     return false;