diff mbox

Fix SLP vectorization of shifts (PR tree-optimization/48616)

Message ID 20110417142614.GW17079@tyan-ft48-01.lab.bos.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek April 17, 2011, 2:26 p.m. UTC 
On Sun, Apr 17, 2011 at 11:30:31AM +0300, Ira Rosen wrote:
> We already have this check in vect_build_slp_tree(). It didn't work for the
> testcase because it doesn't take into account the type of definition. I
> agree that it's better to move it here and base the vector/scalar  decision
> on it, but please remove the now redundant check from vect_build_slp_tree
> ().

I've tried to add this to my patch, unfortunately slp-36.c test then fails
on both x86_64-linux and i686-linux.

The problem is that during the analysis vectorizable_shift is called with
NULL slp_node, but later on it is called with non-NULL:

#0  vect_build_slp_tree (loop_vinfo=0x91dae60, bb_vinfo=0x0, node=0xffffcba8, group_size=2, inside_cost=0xffffcc2c, outside_cost=0xffffcc30, 
    ncopies_for_cost=1, max_nunits=0xffffcc34, load_permutation=0xffffcc38, loads=0xffffcc3c, vectorization_factor=4)
    at ../../gcc/tree-vect-slp.c:428
#1  0x086f9a1f in vect_build_slp_tree (loop_vinfo=0x91dae60, bb_vinfo=0x0, node=0xffffcc28, group_size=2, inside_cost=0xffffcc2c, 
    outside_cost=0xffffcc30, ncopies_for_cost=1, max_nunits=0xffffcc34, load_permutation=0xffffcc38, loads=0xffffcc3c, vectorization_factor=4)
    at ../../gcc/tree-vect-slp.c:648
#2  0x086fae4c in vect_analyze_slp_instance (loop_vinfo=0x91dae60, bb_vinfo=0x0, stmt=0xf7d97900) at ../../gcc/tree-vect-slp.c:1212
#3  0x086fbf17 in vect_analyze_slp (loop_vinfo=0x91dae60, bb_vinfo=0x0) at ../../gcc/tree-vect-slp.c:1303
#4  0x086eef7b in vect_analyze_loop_2 (loop=0xf7d00b7c) at ../../gcc/tree-vect-loop.c:1523
#5  vect_analyze_loop (loop=0xf7d00b7c) at ../../gcc/tree-vect-loop.c:1585

#0  vectorizable_shift (stmt=0xf7d88bc8, gsi=0x0, vec_stmt=0x0, slp_node=0x0) at ../../gcc/tree-vect-stmts.c:2053
#1  0x086e1679 in vect_analyze_stmt (stmt=0xf7d88bc8, need_to_vectorize=0xffffcd08 "\001", node=0x0) at ../../gcc/tree-vect-stmts.c:4719
#2  0x086ef1c9 in vect_analyze_loop_operations (loop=0xf7d00b7c) at ../../gcc/tree-vect-loop.c:1279
#3  vect_analyze_loop_2 (loop=0xf7d00b7c) at ../../gcc/tree-vect-loop.c:1536
#4  vect_analyze_loop (loop=0xf7d00b7c) at ../../gcc/tree-vect-loop.c:1585

#0  vectorizable_shift (stmt=0xf7d88bc8, gsi=0xffffcb80, vec_stmt=0xffffcb0c, slp_node=0x91dbc60) at ../../gcc/tree-vect-stmts.c:2053
#1  0x086e0dec in vect_transform_stmt (stmt=0xf7d88bc8, gsi=0xffffcb80, strided_store=0xffffcb8f "", slp_node=0x91dbc60, 
    slp_node_instance=0x91dbd00) at ../../gcc/tree-vect-stmts.c:4813
#2  0x086f77bf in vect_schedule_slp_instance (node=Unhandled dwarf expression opcode 0xf3) at ../../gcc/tree-vect-slp.c:2494
#3  0x086f765c in vect_schedule_slp_instance (node=0x91dbc08, instance=0x91dbd00, vectorization_factor=2) at ../../gcc/tree-vect-slp.c:2431
#4  0x086fca9f in vect_schedule_slp (loop_vinfo=0x91dae60, bb_vinfo=0x0) at ../../gcc/tree-vect-slp.c:2523
#5  0x086f0687 in vect_transform_loop (loop_vinfo=0x91dae60) at ../../gcc/tree-vect-loop.c:4891
#6  0x086fd685 in vectorize_loops () at ../../gcc/tree-vectorizer.c:205



	Jakub

Comments

Ira Rosen April 18, 2011, 6:11 a.m. UTC | #1 
Jakub Jelinek <jakub@redhat.com> wrote on 17/04/2011 05:26:14 PM:
>
> On Sun, Apr 17, 2011 at 11:30:31AM +0300, Ira Rosen wrote:
> > We already have this check in vect_build_slp_tree(). It didn't work for
the
> > testcase because it doesn't take into account the type of definition. I
> > agree that it's better to move it here and base the vector/scalar
decision
> > on it, but please remove the now redundant check from
vect_build_slp_tree
> > ().
>
> I've tried to add this to my patch, unfortunately slp-36.c test then
fails
> on both x86_64-linux and i686-linux.
>
> The problem is that during the analysis vectorizable_shift is called with
> NULL slp_node, but later on it is called with non-NULL:

Right, sorry, I haven't thought about this. The vect_build_slp_tree check
is done during the analysis and it checks that if vector shift argument is
not supported, the arguments should be equal. And the vectorizable_shift
SLP check is done only during transformation. So, your original patch is
OK.

Thanks,
Ira
diff mbox

Patch

--- gcc/tree-vect-slp.c.jj	2010-12-27 10:18:14.000000000 +0100
+++ gcc/tree-vect-slp.c	2011-04-17 13:06:48.037388449 +0200
@@ -321,12 +321,10 @@  vect_build_slp_tree (loop_vec_info loop_
   enum tree_code first_stmt_code = ERROR_MARK, rhs_code = ERROR_MARK;
   tree first_stmt_def1_type = NULL_TREE, first_stmt_def0_type = NULL_TREE;
   tree lhs;
-  bool stop_recursion = false, need_same_oprnds = false;
-  tree vectype, scalar_type, first_op1 = NULL_TREE;
+  bool stop_recursion = false;
+  tree vectype, scalar_type;
   unsigned int ncopies;
   optab optab;
-  int icode;
-  enum machine_mode optab_op2_mode;
   enum machine_mode vec_mode;
   tree first_stmt_const_oprnd = NULL_TREE;
   struct data_reference *first_dr;
@@ -433,20 +431,6 @@  vect_build_slp_tree (loop_vec_info loop_
 			fprintf (vect_dump, "Build SLP failed: no optab.");
 		      return false;
 		    }
-		  icode = (int) optab_handler (optab, vec_mode);
-		  if (icode == CODE_FOR_nothing)
-		    {
-		      if (vect_print_dump_info (REPORT_SLP))
-			fprintf (vect_dump, "Build SLP failed: "
-				            "op not supported by target.");
-		      return false;
-		    }
-		  optab_op2_mode = insn_data[icode].operand[2].mode;
-		  if (!VECTOR_MODE_P (optab_op2_mode))
-		    {
-		      need_same_oprnds = true;
-		      first_op1 = gimple_assign_rhs2 (stmt);
-		    }
 		}
 	    }
 	}
@@ -472,19 +456,6 @@  vect_build_slp_tree (loop_vec_info loop_
 
 	      return false;
 	    }
-
-	  if (need_same_oprnds
-	      && !operand_equal_p (first_op1, gimple_assign_rhs2 (stmt), 0))
-	    {
-	      if (vect_print_dump_info (REPORT_SLP))
-		{
-		  fprintf (vect_dump,
-			   "Build SLP failed: different shift arguments in ");
-		  print_gimple_stmt (vect_dump, stmt, 0, TDF_SLIM);
-		}
-
-	      return false;
-	    }
 	}
 
       /* Strided store or load.  */