diff mbox

Fix SSA corruption with SLP

Message ID 201103271540.44251.ebotcazou@adacore.com
State New
Headers show

Commit Message

Eric Botcazou March 27, 2011, 1:40 p.m. UTC 
Hi,

the attached testcase exhibits a corruption of the SSA form:

t.c: In function 'foo':
t.c:9:10: error: definition in block 2 follows the use
for SSA_NAME: vect_p.7_12 in statement:
# VUSE <.MEM_6(D)>
vect_var_.8_13 = MEM[(struct R *)vect_p.7_12];
t.c:9:10: internal compiler error: verify_ssa failed
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

introduced by SLP on x86-64 at -O2 -ftree-vectorize -fno-vect-cost-model.

This is a wrong statement rewriting in vectorizable_load.  Before SLP we have:

  D.2687_1 = arg.d2;
  D.2688_2 = arg.d1;
  D.2689_3 = 0.0 - D.2688_2;
  D.2690_4 = D.2687_1 * D.2689_3;
  D.2692.d1 = D.2690_4;

vect_check_interleaving computes that field d1 is accessed before field d2 
because the structure is defined as

struct R {
  double d1;
  double d2;
};

but it's the opposite in the code.  So, in vectorizable_load, first_stmt is the 
load of d1 and new statements are wrongly inserted _after_ the load of d2.
Note that, on release branches (4.5 and 4.6 at least), you get wrong code.

Proposed fix attached.  It adds a GSI parameter to vect_create_data_ref_ptr.
Tested on {i586,x86_64}-suse-linux, OK for the mainline?  And the branches?


2011-03-27  Eric Botcazou  <ebotcazou@adacore.com>

	* tree-vectorizer.h (vect_create_data_ref_ptr): Adjust prototype.
	* tree-vect-data-refs.c (vect_create_data_ref_ptr): Add GSI parameter.
	Insert new statements at it in lieu of STMT.
	(vect_setup_realignment): Adjust call to vect_create_data_ref_ptr.
	* tree-vect-stmts.c (vectorizable_store): Likewise.
	(vectorizable_load): Likewise.


2011-03-27  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc.dg/slp-1.c: New test.

Comments

Ira Rosen March 28, 2011, 7:41 a.m. UTC | #1 
gcc-patches-owner@gcc.gnu.org wrote on 27/03/2011 03:40:44 PM:
>
> Hi,
>
> the attached testcase exhibits a corruption of the SSA form:
>
> t.c: In function 'foo':
> t.c:9:10: error: definition in block 2 follows the use
> for SSA_NAME: vect_p.7_12 in statement:
> # VUSE <.MEM_6(D)>
> vect_var_.8_13 = MEM[(struct R *)vect_p.7_12];
> t.c:9:10: internal compiler error: verify_ssa failed
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <http://gcc.gnu.org/bugs.html> for instructions.
>
> introduced by SLP on x86-64 at -O2 -ftree-vectorize -fno-vect-cost-model.
>
> This is a wrong statement rewriting in vectorizable_load.  Before SLP we
have:
>
>   D.2687_1 = arg.d2;
>   D.2688_2 = arg.d1;
>   D.2689_3 = 0.0 - D.2688_2;
>   D.2690_4 = D.2687_1 * D.2689_3;
>   D.2692.d1 = D.2690_4;
>
> vect_check_interleaving computes that field d1 is accessed before field
d2
> because the structure is defined as
>
> struct R {
>   double d1;
>   double d2;
> };
>
> but it's the opposite in the code.  So, in vectorizable_load,
> first_stmt is the
> load of d1 and new statements are wrongly inserted _after_ the load of
d2.
> Note that, on release branches (4.5 and 4.6 at least), you get wrong
code.
>
> Proposed fix attached.  It adds a GSI parameter to
vect_create_data_ref_ptr.
> Tested on {i586,x86_64}-suse-linux, OK for the mainline?

> Index: tree-vect-data-refs.c
> ===================================================================
> --- tree-vect-data-refs.c	(revision 171572)
> +++ tree-vect-data-refs.c	(working copy)
> @@ -2922,9 +2922,10 @@ vect_create_addr_base_for_vector_ref (gi
>     2. AT_LOOP: the loop where the vector memref is to be created.
>     3. OFFSET (optional): an offset to be added to the initial address
accessed
>          by the data-ref in STMT.
> -   4. ONLY_INIT: indicate if vp is to be updated in the loop, or remain
> +   4. BSI: location where the new stmts are to be placed if there is no
loop

GSI?

> +   5. ONLY_INIT: indicate if vp is to be updated in the loop, or remain
>          pointing to the initial address.
> -   5. TYPE: if not NULL indicates the required type of the data-ref.
> +   6. TYPE: if not NULL indicates the required type of the data-ref.
>
>     Output:
>     1. Declare a new ptr to vector_type, and have it point to the base of
the

OK for the mainline otherwise.

Thanks,
Ira


> And the branches?
>
>
> 2011-03-27  Eric Botcazou  <ebotcazou@adacore.com>
>
>    * tree-vectorizer.h (vect_create_data_ref_ptr): Adjust prototype.
>    * tree-vect-data-refs.c (vect_create_data_ref_ptr): Add GSI parameter.
>    Insert new statements at it in lieu of STMT.
>    (vect_setup_realignment): Adjust call to vect_create_data_ref_ptr.
>    * tree-vect-stmts.c (vectorizable_store): Likewise.
>    (vectorizable_load): Likewise.
>
>
> 2011-03-27  Eric Botcazou  <ebotcazou@adacore.com>
>
>    * gcc.dg/slp-1.c: New test.
>
>
> --
> Eric Botcazou
> [attachment "p.diff" deleted by Ira Rosen/Haifa/IBM] [attachment
> "t.c" deleted by Ira Rosen/Haifa/IBM]
Eric Botcazou March 28, 2011, 7:50 a.m. UTC | #2 
> GSI?

All the other functions of the vectorizer still have BSI in the comments. ;-)

> OK for the mainline otherwise.

Thanks!
diff mbox

Patch

Index: tree-vectorizer.h
===================================================================
--- tree-vectorizer.h	(revision 171572)
+++ tree-vectorizer.h	(working copy)
@@ -824,7 +824,8 @@  extern bool vect_analyze_data_ref_access
 extern bool vect_prune_runtime_alias_test_list (loop_vec_info);
 extern bool vect_analyze_data_refs (loop_vec_info, bb_vec_info, int *);
 extern tree vect_create_data_ref_ptr (gimple, struct loop *, tree, tree *,
-                                      gimple *, bool, bool *);
+                                      gimple_stmt_iterator *, gimple *,
+                                      bool, bool *);
 extern tree bump_vector_ptr (tree, gimple, gimple_stmt_iterator *, gimple, tree);
 extern tree vect_create_destination_var (tree, tree);
 extern bool vect_strided_store_supported (tree);
Index: tree-vect-data-refs.c
===================================================================
--- tree-vect-data-refs.c	(revision 171572)
+++ tree-vect-data-refs.c	(working copy)
@@ -2922,9 +2922,10 @@  vect_create_addr_base_for_vector_ref (gi
    2. AT_LOOP: the loop where the vector memref is to be created.
    3. OFFSET (optional): an offset to be added to the initial address accessed
         by the data-ref in STMT.
-   4. ONLY_INIT: indicate if vp is to be updated in the loop, or remain
+   4. BSI: location where the new stmts are to be placed if there is no loop
+   5. ONLY_INIT: indicate if vp is to be updated in the loop, or remain
         pointing to the initial address.
-   5. TYPE: if not NULL indicates the required type of the data-ref.
+   6. TYPE: if not NULL indicates the required type of the data-ref.
 
    Output:
    1. Declare a new ptr to vector_type, and have it point to the base of the
@@ -2952,9 +2953,9 @@  vect_create_addr_base_for_vector_ref (gi
    4. Return the pointer.  */
 
 tree
-vect_create_data_ref_ptr (gimple stmt, struct loop *at_loop,
-			  tree offset, tree *initial_address, gimple *ptr_incr,
-			  bool only_init, bool *inv_p)
+vect_create_data_ref_ptr (gimple stmt, struct loop *at_loop, tree offset,
+			  tree *initial_address, gimple_stmt_iterator *gsi,
+			  gimple *ptr_incr, bool only_init, bool *inv_p)
 {
   tree base_name;
   stmt_vec_info stmt_info = vinfo_for_stmt (stmt);
@@ -2980,7 +2981,6 @@  vect_create_data_ref_ptr (gimple stmt, s
   gimple incr;
   tree step;
   bb_vec_info bb_vinfo = STMT_VINFO_BB_VINFO (stmt_info);
-  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
   tree base;
 
   if (loop_vinfo)
@@ -3125,7 +3125,7 @@  vect_create_data_ref_ptr (gimple stmt, s
           gcc_assert (!new_bb);
         }
       else
-        gsi_insert_seq_before (&gsi, new_stmt_list, GSI_SAME_STMT);
+        gsi_insert_seq_before (gsi, new_stmt_list, GSI_SAME_STMT);
     }
 
   *initial_address = new_temp;
@@ -3147,7 +3147,7 @@  vect_create_data_ref_ptr (gimple stmt, s
 	  gcc_assert (!new_bb);
 	}
       else
-	gsi_insert_before (&gsi, vec_stmt, GSI_SAME_STMT);
+	gsi_insert_before (gsi, vec_stmt, GSI_SAME_STMT);
     }
   else
     vect_ptr_init = new_temp;
@@ -3672,7 +3672,7 @@  vect_setup_realignment (gimple stmt, gim
       gcc_assert (!compute_in_loop);
       vec_dest = vect_create_destination_var (scalar_dest, vectype);
       ptr = vect_create_data_ref_ptr (stmt, loop_for_initial_load, NULL_TREE,
-				      &init_addr, &inc, true, &inv_p);
+				      &init_addr, NULL, &inc, true, &inv_p);
       new_stmt = gimple_build_assign_with_ops
 		   (BIT_AND_EXPR, NULL_TREE, ptr,
 		    build_int_cst (TREE_TYPE (ptr),
Index: tree-vect-stmts.c
===================================================================
--- tree-vect-stmts.c	(revision 171572)
+++ tree-vect-stmts.c	(working copy)
@@ -3582,7 +3582,7 @@  vectorizable_store (gimple stmt, gimple_
 	  gcc_assert (useless_type_conversion_p (vectype,
 						 TREE_TYPE (vec_oprnd)));
 	  dataref_ptr = vect_create_data_ref_ptr (first_stmt, NULL, NULL_TREE,
-						  &dummy, &ptr_incr, false,
+						  &dummy, gsi, &ptr_incr, false,
 						  &inv_p);
 	  gcc_assert (bb_vinfo || !inv_p);
 	}
@@ -4109,9 +4109,8 @@  vectorizable_load (gimple stmt, gimple_s
     {
       /* 1. Create the vector pointer update chain.  */
       if (j == 0)
-        dataref_ptr = vect_create_data_ref_ptr (first_stmt,
-					        at_loop, offset,
-						&dummy, &ptr_incr, false,
+        dataref_ptr = vect_create_data_ref_ptr (first_stmt, at_loop, offset,
+						&dummy, gsi, &ptr_incr, false,
 						&inv_p);
       else
         dataref_ptr =