Re: [PATCH] Fix PR64829

[Kyrill Tkachov]

On 29/01/15 12:44, Richard Biener wrote:
On Wed, 28 Jan 2015, Kyrill Tkachov wrote:
Hi Richard,

On 28/01/15 14:14, Richard Biener wrote:
This fixes PR64829 where widening shift pattern detection fails to
verify the widening operation is used only in the shift.

Bootstrap and regtest running on x86_64-unknown-linux-gnu.
This patch causes a testsuite fail on an arm-none-eabi cross:
FAIL: gcc.dg/vect/vect-widen-shift-u8.c scan-tree-dump-times vect
"vect_recog_widen_shift_pattern: detected" 2
FAIL: gcc.dg/vect/vect-widen-shift-u8.c -flto -ffat-lto-objects
scan-tree-dump-times vect "vect_recog_widen_shift_pattern: detected" 2

The widen_shift pattern doesn't get recognised although the loop is still
vectorised.
Is this something we can avoid?
Or should the test be adjusted?
Hmm, I'm looking at it closer now - the issue is that the code tries
to handle multiple widened shifts using the same pattern stmt (sth
we generally fail to do - see bool pattern handling for a good example).
But it doesn't consider uses that will not end up in the widen-shift
patterns.

But then we don't support vectorizing a statement with both using
the original stmt and its pattern replacement (still somehow the
code dealing with life analysis tries to handle it, but in a very
weird non-working way - see vect_mark_relevant in the
STMT_VINFO_IN_PATTERN_P && !used_in_pattern case.

The widening multiplication pattern detection has the very same issue.

I don't see an easy (and thus non-ugly) way out here apart from
_not_ marking the extra conversion we insert as related stmt to
the existing widening (but then we'll not be able to re-use that
stmt as far as I can see - not easily at least without hacking
pattern recog to add some on-the-side info).  life analysis should
then determine correctly whether the original widening is still
life (uses both in patterns and non-patterns) or if it can be
discarded.

This if course delays CSE of the intermediate vec_unpack_lo/hi_exprs.
But it handles the mixed uses - widening multiplication has own
handling that is correct, just the one it inherits from
vect_handle_widen_op_by_const is broken.

Can you test?

This version fixes the ICE in the PR and the testsuite runs looks ok too.
Minor nit,  the definition of new_stmt in vect_handle_widen_op_by_const 
should be removed to avoid an 'unused variable' warning.
Jiong (or somebody else), could you please bootstrap this on arm when 
you get the chance? I'll be offline in the next couple of days.

Thanks,
Kyrill


Thanks,
Richard.

Index: gcc/tree-vect-patterns.c
===================================================================
--- gcc/tree-vect-patterns.c    (revision 220205)
+++ gcc/tree-vect-patterns.c    (working copy)
@@ -726,7 +726,7 @@ vect_recog_sad_pattern (vec<gimple> *stm
  static bool
  vect_handle_widen_op_by_const (gimple stmt, enum tree_code code,
                               tree const_oprnd, tree *oprnd,
-                              vec<gimple> *stmts, tree type,
+                              gimple *wstmt, tree type,
                               tree *half_type, gimple def_stmt)
  {
    tree new_type, new_oprnd;
@@ -761,29 +761,11 @@ vect_handle_widen_op_by_const (gimple st
            && compare_tree_int (const_oprnd, TYPE_PRECISION (new_type)) == 1))
      return false;
  
-  /* Use NEW_TYPE for widening operation.  */
-  if (STMT_VINFO_RELATED_STMT (vinfo_for_stmt (def_stmt)))
-    {
-      new_stmt = STMT_VINFO_RELATED_STMT (vinfo_for_stmt (def_stmt));
-      /* Check if the already created pattern stmt is what we need.  */
-      if (!is_gimple_assign (new_stmt)
-          || !CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (new_stmt))
-          || TREE_TYPE (gimple_assign_lhs (new_stmt)) != new_type)
-        return false;
-
-      stmts->safe_push (def_stmt);
-      *oprnd = gimple_assign_lhs (new_stmt);
-    }
-  else
-    {
-      /* Create a_T = (NEW_TYPE) a_t;  */
-      *oprnd = gimple_assign_rhs1 (def_stmt);
-      new_oprnd = make_ssa_name (new_type);
-      new_stmt = gimple_build_assign (new_oprnd, NOP_EXPR, *oprnd);
-      STMT_VINFO_RELATED_STMT (vinfo_for_stmt (def_stmt)) = new_stmt;
-      stmts->safe_push (def_stmt);
-      *oprnd = new_oprnd;
-    }
+  /* Use NEW_TYPE for widening operation and create a_T = (NEW_TYPE) a_t;  */
+  *oprnd = gimple_assign_rhs1 (def_stmt);
+  new_oprnd = make_ssa_name (new_type);
+  *wstmt = gimple_build_assign (new_oprnd, NOP_EXPR, *oprnd);
+  *oprnd = new_oprnd;
  
    *half_type = new_type;
    return true;
@@ -920,7 +902,7 @@ vect_recog_widen_mult_pattern (vec<gimpl
        if (TREE_CODE (oprnd1) == INTEGER_CST
            && TREE_CODE (half_type0) == INTEGER_TYPE
            && vect_handle_widen_op_by_const (last_stmt, MULT_EXPR, oprnd1,
-                                           &oprnd0, stmts, type,
+                                           &oprnd0, &new_stmt, type,
                                            &half_type0, def_stmt0))
        {
          half_type1 = half_type0;
@@ -934,6 +916,10 @@ vect_recog_widen_mult_pattern (vec<gimpl
       the smaller type into the larger type.  */
    if (TYPE_PRECISION (half_type0) != TYPE_PRECISION (half_type1))
      {
+      /* If we already used up the single-stmt slot give up.  */
+      if (new_stmt)
+       return NULL;
+
        tree* oprnd = NULL;
        gimple def_stmt = NULL;
  
@@ -1734,7 +1720,7 @@ vect_recog_widen_shift_pattern (vec<gimp
  
    /* Check operand 0: it has to be defined by a type promotion.  */
    if (!type_conversion_p (oprnd0, last_stmt, false, &half_type0, &def_stmt0,
-                          &promotion)
+                         &promotion)
        || !promotion)
       return NULL;
  
@@ -1764,8 +1750,9 @@ vect_recog_widen_shift_pattern (vec<gimp
      }
  
    /* Check if this a widening operation.  */
+  gimple wstmt = NULL;
    if (!vect_handle_widen_op_by_const (last_stmt, LSHIFT_EXPR, oprnd1,
-                                             &oprnd0, stmts,
+                                             &oprnd0, &wstmt,
                                      type, &half_type0, def_stmt0))
      return NULL;
  
@@ -1793,6 +1780,17 @@ vect_recog_widen_shift_pattern (vec<gimp
    var = vect_recog_temp_ssa_var (type, NULL);
    pattern_stmt =
      gimple_build_assign (var, WIDEN_LSHIFT_EXPR, oprnd0, oprnd1);
+  if (wstmt)
+    {
+      stmt_vec_info stmt_vinfo = vinfo_for_stmt (last_stmt);
+      loop_vec_info loop_vinfo = STMT_VINFO_LOOP_VINFO (stmt_vinfo);
+      bb_vec_info bb_vinfo = STMT_VINFO_BB_VINFO (stmt_vinfo);
+      new_pattern_def_seq (stmt_vinfo, wstmt);
+      stmt_vec_info new_stmt_info
+       = new_stmt_vec_info (wstmt, loop_vinfo, bb_vinfo);
+      set_vinfo_for_stmt (wstmt, new_stmt_info);
+      STMT_VINFO_VECTYPE (new_stmt_info) = vectype;
+    }
  
    if (dump_enabled_p ())
      dump_gimple_stmt_loc (MSG_NOTE, vect_location, TDF_SLIM, pattern_stmt, 0);
@@ -3414,7 +3412,6 @@ vect_pattern_recog_1 (vect_recog_func_pt
        dump_printf_loc (MSG_NOTE, vect_location,
                         "pattern recognized: ");
        dump_gimple_stmt (MSG_NOTE, TDF_SLIM, pattern_stmt, 0);
-      dump_printf (MSG_NOTE, "\n");
      }
  
    /* Mark the stmts that are involved in the pattern. */
@@ -3441,7 +3438,6 @@ vect_pattern_recog_1 (vect_recog_func_pt
            dump_printf_loc (MSG_NOTE, vect_location,
                             "additional pattern stmt: ");
            dump_gimple_stmt (MSG_NOTE, TDF_SLIM, pattern_stmt, 0);
-          dump_printf (MSG_NOTE, "\n");
          }
  
        vect_mark_pattern_stmts (stmt, pattern_stmt, NULL_TREE);