diff mbox

Eliminate vectorizer analysis side effects

Message ID CAAkRFZLS+=6c7vkVXasHfDh+fbcmuJXeTgbMgZuX7eL-cdNqyg@mail.gmail.com
State New
Headers show

Commit Message

Xinliang David Li Aug. 29, 2013, 11:28 p.m. UTC 
I was debugging a runtime failure of SPEC06 xalancbmk built with LIPO.
Using -fdisable-<pass> option pinpoints the problem in slp vectorize
pass on a particular function. dbgcnt support is added to to track
down the individual BB, but it  fails even when the dbg count is set
to 0.

It turns out that no BB was actually vectorized for that function, but
turning on/off slp-vectorize does make a difference in generated code
-- the only difference between the good and bad case is stack layout.
 The problem is  in the alignment analysis phase -- which
speculatively changes the base declaration's alignment regardless
whether the vectorization transformation will be performed or not
later.

The attached patch fixes the problem. Testing is ok. Ok for trunk?

thanks,

David

Comments

Richard Biener Aug. 30, 2013, 8:23 a.m. UTC | #1 
On Fri, Aug 30, 2013 at 1:28 AM, Xinliang David Li <davidxl@google.com> wrote:
> I was debugging a runtime failure of SPEC06 xalancbmk built with LIPO.
> Using -fdisable-<pass> option pinpoints the problem in slp vectorize
> pass on a particular function. dbgcnt support is added to to track
> down the individual BB, but it  fails even when the dbg count is set
> to 0.
>
> It turns out that no BB was actually vectorized for that function, but
> turning on/off slp-vectorize does make a difference in generated code
> -- the only difference between the good and bad case is stack layout.
>  The problem is  in the alignment analysis phase -- which
> speculatively changes the base declaration's alignment regardless
> whether the vectorization transformation will be performed or not
> later.
>
> The attached patch fixes the problem. Testing is ok. Ok for trunk?

Not in this form.  I'd rather not put extra fields in the data-refs this way.
(As for the xalancbmk runtime problem - doesn't this patch just paper
over the real issue?)

For BB SLP you still adjust DR bases that do not take part in
vectorization - the DR vector contains all refs in the BB, not just
those in the SLP instances we are going to vectorize.  So the
commit of the re-aligning decision should be done from where
we vectorize the DR, in vectorizable_load/store in its transform
phase.

If we decide to integrate the fields into the data-ref then the
analysis and commit parts should go into the data-ref machinery
as well.  Otherwise the vectorizer should use data-ref->aux or some
other way to hang off its private data.

Other than that, modifying alignment of variables that are not
vectorized is indeed a bug worth fixing.

Richard.

> thanks,
>
> David
diff mbox

Patch

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 202088)
+++ ChangeLog	(working copy)
@@ -1,5 +1,17 @@ 
 2013-08-29  Xinliang David Li  <davidxl@google.com>
 
+	* tree-vect-data-refs.c (vect_compute_data_ref_alignment):
+	Delay base decl alignment adjustment.
+	* tree-vectorizer.c (ensure_base_alignment): New function.
+	(vectorize_loops): Add dbg_cnt support. Perform alignment
+	adjustment.
+	(execute_vect_slp): Ditto.
+	* dbgcnt.def: New debug counter.
+	* tree-data-ref.h: New fields.
+	* Makefile: New dependency.
+
+2013-08-29  Xinliang David Li  <davidxl@google.com>
+
 	* loop-unroll.c (report_unroll_peel): Minor message
 	change.
 	* tree-vect-loop-manip.c (vect_do_peeling_for_alignment):
Index: tree-vect-data-refs.c
===================================================================
--- tree-vect-data-refs.c	(revision 202088)
+++ tree-vect-data-refs.c	(working copy)
@@ -763,15 +763,10 @@  vect_compute_data_ref_alignment (struct
           dump_generic_expr (MSG_NOTE, TDF_SLIM, ref);
         }
 
-      DECL_ALIGN (base) = TYPE_ALIGN (vectype);
-      DECL_USER_ALIGN (base) = 1;
+      DR_BASE_DECL (dr) = base;
+      DR_BASE_MISALIGNED (dr) = true;
     }
 
-  /* At this point we assume that the base is aligned.  */
-  gcc_assert (base_aligned
-	      || (TREE_CODE (base) == VAR_DECL
-		  && DECL_ALIGN (base) >= TYPE_ALIGN (vectype)));
-
   /* If this is a backward running DR then first access in the larger
      vectype actually is N-1 elements before the address in the DR.
      Adjust misalign accordingly.  */
Index: dbgcnt.def
===================================================================
--- dbgcnt.def	(revision 202088)
+++ dbgcnt.def	(working copy)
@@ -172,6 +172,8 @@  DEBUG_COUNTER (pre_insn)
 DEBUG_COUNTER (treepre_insert)
 DEBUG_COUNTER (tree_sra)
 DEBUG_COUNTER (eipa_sra)
+DEBUG_COUNTER (vect_loop)
+DEBUG_COUNTER (vect_slp)
 DEBUG_COUNTER (sched2_func)
 DEBUG_COUNTER (sched_block)
 DEBUG_COUNTER (sched_func)
Index: tree-vectorizer.c
===================================================================
--- tree-vectorizer.c	(revision 202088)
+++ tree-vectorizer.c	(working copy)
@@ -68,6 +68,7 @@  along with GCC; see the file COPYING3.
 #include "tree-pass.h"
 #include "hash-table.h"
 #include "tree-ssa-propagate.h"
+#include "dbgcnt.h"
 
 /* Loop or bb location.  */
 LOC vect_location;
@@ -279,6 +280,37 @@  note_simd_array_uses (hash_table <simd_a
       }
 }
 
+/* A helper function to enforce base alignment
+   before transforamtion.  */
+
+static void
+ensure_base_alignment (loop_vec_info loop_vinfo, bb_vec_info bb_vinfo)
+{
+  vec<data_reference_p> datarefs;
+  struct data_reference *dr;
+  unsigned int i;
+
+ if (loop_vinfo)
+    datarefs = LOOP_VINFO_DATAREFS (loop_vinfo);
+  else
+    datarefs = BB_VINFO_DATAREFS (bb_vinfo);
+
+  FOR_EACH_VEC_ELT (datarefs, i, dr)
+    {
+      tree base_decl = DR_BASE_DECL (dr);
+      if (base_decl && DR_BASE_MISALIGNED (dr))
+        {
+          gimple stmt = DR_STMT (dr);
+          stmt_vec_info stmt_info = vinfo_for_stmt (stmt);
+          tree vectype = STMT_VINFO_VECTYPE (stmt_info);
+          DECL_ALIGN (base_decl) = TYPE_ALIGN (vectype);
+          DECL_USER_ALIGN (base_decl) = 1;
+          DR_BASE_MISALIGNED (dr) = false;
+        }
+    }
+}
+
+
 /* Function vectorize_loops.
 
    Entry point to loop vectorization phase.  */
@@ -331,10 +363,14 @@  vectorize_loops (void)
 	if (!loop_vinfo || !LOOP_VINFO_VECTORIZABLE_P (loop_vinfo))
 	  continue;
 
+        if (!dbg_cnt (vect_loop))
+	  break;
+
         if (LOCATION_LOCUS (vect_location) != UNKNOWN_LOC
 	    && dump_enabled_p ())
           dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, vect_location,
                            "loop vectorized\n");
+        ensure_base_alignment (loop_vinfo, NULL);
 	vect_transform_loop (loop_vinfo);
 	num_vectorized_loops++;
 	/* Now that the loop has been vectorized, allow it to be unrolled
@@ -431,6 +467,7 @@  static unsigned int
 execute_vect_slp (void)
 {
   basic_block bb;
+  bb_vec_info bb_vinfo;
 
   init_stmt_vec_info_vec ();
 
@@ -438,8 +475,12 @@  execute_vect_slp (void)
     {
       vect_location = find_bb_location (bb);
 
-      if (vect_slp_analyze_bb (bb))
+      if ((bb_vinfo = vect_slp_analyze_bb (bb)))
         {
+	  if (!dbg_cnt (vect_slp))
+	    break;
+
+          ensure_base_alignment (NULL, bb_vinfo);
           vect_slp_transform_bb (bb);
           if (dump_enabled_p ())
             dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, vect_location,
Index: tree-data-ref.h
===================================================================
--- tree-data-ref.h	(revision 202088)
+++ tree-data-ref.h	(working copy)
@@ -176,6 +176,12 @@  struct data_reference
   /* Auxiliary info specific to a pass.  */
   void *aux;
 
+  /* Base var decl.  */
+  tree base_decl;
+
+  /* A boolean indicating if the base decl is misaligned.  */
+  bool base_misaligned;
+
   /* True when the data reference is in RHS of a stmt.  */
   bool is_read;
 
@@ -193,6 +199,8 @@  struct data_reference
 };
 
 #define DR_STMT(DR)                (DR)->stmt
+#define DR_BASE_DECL(DR)           (DR)->base_decl
+#define DR_BASE_MISALIGNED(DR)     (DR)->base_misaligned
 #define DR_REF(DR)                 (DR)->ref
 #define DR_BASE_OBJECT(DR)         (DR)->indices.base_object
 #define DR_UNCONSTRAINED_BASE(DR)  (DR)->indices.unconstrained_base