diff mbox

Fix PR tree-optimization/71170

Message ID CAELXzTOxTWx3Ti20H23x0k4_f7ys_KMbPMF=uBMiqutjJcMS9Q@mail.gmail.com
State Superseded
Headers show

Commit Message

Kugan Vivekanandarajah May 19, 2016, 11:51 p.m. UTC
Hi Richard,

> I think it should have the same rank as op or op + 1 which is the current

> behavior.  Sth else doesn't work correctly here I think, like inserting the

> multiplication not near the definition of op.

>

> Well, the whole "clever insertion" logic is simply flawed.


What I meant to say was that the simple logic we have now wouldn’t
work. "clever logic" is knowing where exactly where it is needed and
inserting there.  I think thats what  you are suggesting below in a
simple to implement way.

> I'd say that ideally we would delay inserting the multiplication to

> rewrite_expr_tree time.  For example by adding a ops->stmt_to_insert

> member.

>


Here is an implementation based on above. Bootstrap on x86-linux-gnu
is OK. regression testing is ongoing.

Thanks,
Kugan

gcc/ChangeLog:

2016-05-20  Kugan Vivekanandarajah  <kugan.vivekanandarajah@linaro.org>

    * tree-ssa-reassoc.c (struct operand_entry): Add field stmt_to_insert.
    (add_to_ops_vec): Add stmt_to_insert.
    (add_repeat_to_ops_vec): Init stmt_to_insert.
    (transform_add_to_multiply): Remove mult_stmt insertion and add it
to ops vector.
    (get_ops): Init stmt_to_insert.
    (maybe_optimize_range_tests): Likewise.
    (rewrite_expr_tree): Insert  stmt_to_insert before use stmt.
    (rewrite_expr_tree_parallel): Likewise.
diff mbox

Patch

diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 3b5f36b..69441ce 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -195,6 +195,7 @@  struct operand_entry
   int id;
   tree op;
   unsigned int count;
+  gimple *stmt_to_insert;
 };
 
 static object_allocator<operand_entry> operand_entry_pool
@@ -553,7 +554,7 @@  sort_by_operand_rank (const void *pa, const void *pb)
 /* Add an operand entry to *OPS for the tree operand OP.  */
 
 static void
-add_to_ops_vec (vec<operand_entry *> *ops, tree op)
+add_to_ops_vec (vec<operand_entry *> *ops, tree op, gimple *stmt_to_insert = NULL)
 {
   operand_entry *oe = operand_entry_pool.allocate ();
 
@@ -561,6 +562,7 @@  add_to_ops_vec (vec<operand_entry *> *ops, tree op)
   oe->rank = get_rank (op);
   oe->id = next_operand_entry_id++;
   oe->count = 1;
+  oe->stmt_to_insert = stmt_to_insert;
   ops->safe_push (oe);
 }
 
@@ -577,6 +579,7 @@  add_repeat_to_ops_vec (vec<operand_entry *> *ops, tree op,
   oe->rank = get_rank (op);
   oe->id = next_operand_entry_id++;
   oe->count = repeat;
+  oe->stmt_to_insert = NULL;
   ops->safe_push (oe);
 
   reassociate_stats.pows_encountered++;
@@ -1810,21 +1813,12 @@  transform_add_to_multiply (gimple *stmt, vec<operand_entry *> *ops)
 	ops->unordered_remove (i);
       tree tmp = make_ssa_name (TREE_TYPE (op));
       tree cst = build_int_cst (integer_type_node, count);
-      gimple *def_stmt = SSA_NAME_DEF_STMT (op);
       gassign *mul_stmt
 	= gimple_build_assign (tmp, MULT_EXPR,
 			       op, fold_convert (TREE_TYPE (op), cst));
-      if (gimple_code (def_stmt) == GIMPLE_NOP
-	  || gimple_bb (stmt) != gimple_bb (def_stmt))
-	{
-	  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
-	  gimple_set_uid (mul_stmt, gimple_uid (stmt));
-	  gsi_insert_before (&gsi, mul_stmt, GSI_NEW_STMT);
-	}
-      else
-	insert_stmt_after (mul_stmt, def_stmt);
+      gimple_set_uid (mul_stmt, gimple_uid (stmt));
       gimple_set_visited (mul_stmt, true);
-      add_to_ops_vec (ops, tmp);
+      add_to_ops_vec (ops, tmp, mul_stmt);
       changed = true;
     }
 
@@ -3224,6 +3218,7 @@  get_ops (tree var, enum tree_code code, vec<operand_entry *> *ops,
 	oe->rank = code;
 	oe->id = 0;
 	oe->count = 1;
+	oe->stmt_to_insert = NULL;
 	ops->safe_push (oe);
       }
   return true;
@@ -3464,6 +3459,7 @@  maybe_optimize_range_tests (gimple *stmt)
 	      oe->rank = code;
 	      oe->id = 0;
 	      oe->count = 1;
+	      oe->stmt_to_insert = NULL;
 	      ops.safe_push (oe);
 	      bb_ent.last_idx++;
 	    }
@@ -3501,6 +3497,7 @@  maybe_optimize_range_tests (gimple *stmt)
 	     is.  */
 	  oe->id = bb->index;
 	  oe->count = 1;
+	  oe->stmt_to_insert = NULL;
 	  ops.safe_push (oe);
 	  bb_ent.op = NULL;
 	  bb_ent.last_idx++;
@@ -3798,6 +3795,19 @@  rewrite_expr_tree (gimple *stmt, unsigned int opindex,
       oe1 = ops[opindex];
       oe2 = ops[opindex + 1];
 
+      /* If the stmt that defines operand has to be inserted, insert it
+	 before the use.  */
+      if (oe1->stmt_to_insert)
+	{
+	  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
+	  gsi_insert_before (&gsi, oe1->stmt_to_insert, GSI_NEW_STMT);
+	}
+      if (oe2->stmt_to_insert)
+	{
+	  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
+	  gsi_insert_before (&gsi, oe2->stmt_to_insert, GSI_NEW_STMT);
+	}
+
       if (rhs1 != oe1->op || rhs2 != oe2->op)
 	{
 	  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
@@ -3855,6 +3865,14 @@  rewrite_expr_tree (gimple *stmt, unsigned int opindex,
   /* Rewrite the next operator.  */
   oe = ops[opindex];
 
+  /* If the stmt that defines operand has to be inserted, insert it
+     before the use.  */
+  if (oe->stmt_to_insert)
+    {
+      gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
+      gsi_insert_before (&gsi, oe->stmt_to_insert, GSI_NEW_STMT);
+    }
+
   /* Recurse on the LHS of the binary operator, which is guaranteed to
      be the non-leaf side.  */
   tree new_rhs1
@@ -3999,6 +4017,7 @@  rewrite_expr_tree_parallel (gassign *stmt, int width,
   int stmt_index = 0;
   int ready_stmts_end = 0;
   int i = 0;
+  gimple *stmt1 = NULL, *stmt2 = NULL;
   tree last_rhs1 = gimple_assign_rhs1 (stmt);
 
   /* We start expression rewriting from the top statements.
@@ -4027,7 +4046,11 @@  rewrite_expr_tree_parallel (gassign *stmt, int width,
 	  if (ready_stmts_end > stmt_index)
 	    op2 = gimple_assign_lhs (stmts[stmt_index++]);
 	  else if (op_index >= 0)
-	    op2 = ops[op_index--]->op;
+	    {
+	      operand_entry *oe = ops[op_index--];
+	      stmt2 = oe->stmt_to_insert;
+	      op2 = oe->op;
+	    }
 	  else
 	    {
 	      gcc_assert (stmt_index < i);
@@ -4041,8 +4064,12 @@  rewrite_expr_tree_parallel (gassign *stmt, int width,
 	{
 	  if (op_index > 1)
 	    swap_ops_for_binary_stmt (ops, op_index - 2, NULL);
-	  op2 = ops[op_index--]->op;
-	  op1 = ops[op_index--]->op;
+	  operand_entry *oe2 = ops[op_index--];
+	  operand_entry *oe1 = ops[op_index--];
+	  op2 = oe2->op;
+	  stmt2 = oe2->stmt_to_insert;
+	  op1 = oe1->op;
+	  stmt1 = oe1->stmt_to_insert;
 	}
 
       /* If we emit the last statement then we should put
@@ -4057,6 +4084,19 @@  rewrite_expr_tree_parallel (gassign *stmt, int width,
 	  print_gimple_stmt (dump_file, stmts[i], 0, 0);
 	}
 
+      /* If the stmt that defines operand has to be inserted, insert it
+	 before the use.  */
+      if (stmt1)
+	{
+	  gimple_stmt_iterator gsi = gsi_for_stmt (stmts[i]);
+	  gsi_insert_before (&gsi, stmt1, GSI_NEW_STMT);
+	}
+      if (stmt2)
+	{
+	  gimple_stmt_iterator gsi = gsi_for_stmt (stmts[i]);
+	  gsi_insert_before (&gsi, stmt2, GSI_NEW_STMT);
+	}
+
       /* We keep original statement only for the last one.  All
 	 others are recreated.  */
       if (i == stmt_num - 1)