Re: [PR71252][PR71269] Fix trunk errors due to stmt_to_insert

[Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

Hi Jakub,


On 26 May 2016 at 18:18, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, May 26, 2016 at 02:17:56PM +1000, Kugan Vivekanandarajah wrote:
>> --- a/gcc/tree-ssa-reassoc.c
>> +++ b/gcc/tree-ssa-reassoc.c
>> @@ -3767,8 +3767,10 @@ swap_ops_for_binary_stmt (vec<operand_entry *> ops,
>>        operand_entry temp = *oe3;
>>        oe3->op = oe1->op;
>>        oe3->rank = oe1->rank;
>> +      oe3->stmt_to_insert = oe1->stmt_to_insert;
>>        oe1->op = temp.op;
>>        oe1->rank= temp.rank;
>> +      oe1->stmt_to_insert = temp.stmt_to_insert;
>
> If you want to swap those 3 fields (what about the others?), can't you write
>       std::swap (oe1->op, oe3->op);
>       std::swap (oe1->rank, oe3->rank);
>       std::swap (oe1->stmt_to_insert, oe3->stmt_to_insert);
> instead and drop operand_entry temp = *oe3; ?
>
>>      }
>>    else if ((oe1->rank == oe3->rank
>>           && oe2->rank != oe3->rank)
>> @@ -3779,8 +3781,10 @@ swap_ops_for_binary_stmt (vec<operand_entry *> ops,
>>        operand_entry temp = *oe2;
>>        oe2->op = oe1->op;
>>        oe2->rank = oe1->rank;
>> +      oe2->stmt_to_insert = oe1->stmt_to_insert;
>>        oe1->op = temp.op;
>>        oe1->rank = temp.rank;
>> +      oe1->stmt_to_insert = temp.stmt_to_insert;
>>      }
>
> Similarly.

Done. Revised patch attached.

Thanks,
Kugan

[-- Attachment #2: pr71252_2.txt --]
[-- Type: text/plain, Size: 8237 bytes --]

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr71269.c b/gcc/testsuite/gcc.dg/tree-ssa/pr71269.c
index e69de29..4dceaaa 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr71269.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr71269.c
@@ -0,0 +1,10 @@
+/* PR middle-end/71269 */
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+
+int a, b, c;
+void  fn2 (int);
+void fn1 ()
+{
+  fn2 (sizeof 0 + c + a + b + b);
+}
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index c9ed679..db6ac6b 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -3764,11 +3764,9 @@ swap_ops_for_binary_stmt (vec<operand_entry *> ops,
 	  && !is_phi_for_stmt (stmt, oe1->op)
 	  && !is_phi_for_stmt (stmt, oe2->op)))
     {
-      operand_entry temp = *oe3;
-      oe3->op = oe1->op;
-      oe3->rank = oe1->rank;
-      oe1->op = temp.op;
-      oe1->rank= temp.rank;
+      std::swap (oe1->op, oe3->op);
+      std::swap (oe1->rank, oe3->rank);
+      std::swap (oe1->stmt_to_insert, oe3->stmt_to_insert);
     }
   else if ((oe1->rank == oe3->rank
 	    && oe2->rank != oe3->rank)
@@ -3776,11 +3774,9 @@ swap_ops_for_binary_stmt (vec<operand_entry *> ops,
 	       && !is_phi_for_stmt (stmt, oe1->op)
 	       && !is_phi_for_stmt (stmt, oe3->op)))
     {
-      operand_entry temp = *oe2;
-      oe2->op = oe1->op;
-      oe2->rank = oe1->rank;
-      oe1->op = temp.op;
-      oe1->rank = temp.rank;
+      std::swap (oe1->op, oe2->op);
+      std::swap (oe1->rank, oe2->rank);
+      std::swap (oe1->stmt_to_insert, oe2->stmt_to_insert);
     }
 }
 
@@ -3790,6 +3786,42 @@ swap_ops_for_binary_stmt (vec<operand_entry *> ops,
 static inline gimple *
 find_insert_point (gimple *stmt, tree rhs1, tree rhs2)
 {
+  /* If rhs1 is defined by stmt_to_insert, insert after its argument
+     definion stmt.  */
+  if (TREE_CODE (rhs1) == SSA_NAME
+      && !gimple_nop_p (SSA_NAME_DEF_STMT (rhs1))
+      && !gimple_bb (SSA_NAME_DEF_STMT (rhs1)))
+    {
+      gimple *stmt1 = SSA_NAME_DEF_STMT (rhs1);
+      gcc_assert (is_gimple_assign (stmt1));
+      tree rhs11 = gimple_assign_rhs1 (stmt1);
+      tree rhs12 = gimple_assign_rhs2 (stmt1);
+      if (TREE_CODE (rhs11) == SSA_NAME
+	  && reassoc_stmt_dominates_stmt_p (stmt, SSA_NAME_DEF_STMT (rhs11)))
+	stmt = SSA_NAME_DEF_STMT (rhs11);
+      if (TREE_CODE (rhs12) == SSA_NAME
+	  && reassoc_stmt_dominates_stmt_p (stmt, SSA_NAME_DEF_STMT (rhs12)))
+	stmt = SSA_NAME_DEF_STMT (rhs12);
+    }
+
+  /* If rhs2 is defined by stmt_to_insert, insert after its argument
+     definion stmt.  */
+  if (TREE_CODE (rhs2) == SSA_NAME
+      && !gimple_nop_p (SSA_NAME_DEF_STMT (rhs2))
+      && !gimple_bb (SSA_NAME_DEF_STMT (rhs2)))
+    {
+      gimple *stmt1 = SSA_NAME_DEF_STMT (rhs2);
+      gcc_assert (is_gimple_assign (stmt1));
+      tree rhs11 = gimple_assign_rhs1 (stmt1);
+      tree rhs12 = gimple_assign_rhs2 (stmt1);
+      if (TREE_CODE (rhs11) == SSA_NAME
+	  && reassoc_stmt_dominates_stmt_p (stmt, SSA_NAME_DEF_STMT (rhs11)))
+	stmt = SSA_NAME_DEF_STMT (rhs11);
+      if (TREE_CODE (rhs12) == SSA_NAME
+	  && reassoc_stmt_dominates_stmt_p (stmt, SSA_NAME_DEF_STMT (rhs12)))
+	stmt = SSA_NAME_DEF_STMT (rhs12);
+    }
+
   if (TREE_CODE (rhs1) == SSA_NAME
       && reassoc_stmt_dominates_stmt_p (stmt, SSA_NAME_DEF_STMT (rhs1)))
     stmt = SSA_NAME_DEF_STMT (rhs1);
@@ -3843,12 +3875,6 @@ rewrite_expr_tree (gimple *stmt, unsigned int opindex,
 	    {
 	      gimple *insert_point
 		= find_insert_point (stmt, oe1->op, oe2->op);
-	      /* If the stmt that defines operand has to be inserted, insert it
-		 before the use.  */
-	      if (oe1->stmt_to_insert)
-		insert_stmt_before_use (stmt, oe1->stmt_to_insert);
-	      if (oe2->stmt_to_insert)
-		insert_stmt_before_use (stmt, oe2->stmt_to_insert);
 	      lhs = make_ssa_name (TREE_TYPE (lhs));
 	      stmt
 		= gimple_build_assign (lhs, gimple_assign_rhs_code (stmt),
@@ -3864,17 +3890,17 @@ rewrite_expr_tree (gimple *stmt, unsigned int opindex,
 	    {
 	      gcc_checking_assert (find_insert_point (stmt, oe1->op, oe2->op)
 				   == stmt);
-	      /* If the stmt that defines operand has to be inserted, insert it
-		 before the use.  */
-	      if (oe1->stmt_to_insert)
-		insert_stmt_before_use (stmt, oe1->stmt_to_insert);
-	      if (oe2->stmt_to_insert)
-		insert_stmt_before_use (stmt, oe2->stmt_to_insert);
 	      gimple_assign_set_rhs1 (stmt, oe1->op);
 	      gimple_assign_set_rhs2 (stmt, oe2->op);
 	      update_stmt (stmt);
 	    }
 
+	  /* If the stmt that defines operand has to be inserted, insert it
+	     before the use after the stmt is inserted.  */
+	  if (oe1->stmt_to_insert)
+	    insert_stmt_before_use (stmt, oe1->stmt_to_insert);
+	  if (oe2->stmt_to_insert)
+	    insert_stmt_before_use (stmt, oe2->stmt_to_insert);
 	  if (rhs1 != oe1->op && rhs1 != oe2->op)
 	    remove_visited_stmt_chain (rhs1);
 
@@ -3893,11 +3919,6 @@ 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)
-    insert_stmt_before_use (stmt, oe->stmt_to_insert);
-
   /* Recurse on the LHS of the binary operator, which is guaranteed to
      be the non-leaf side.  */
   tree new_rhs1
@@ -3944,6 +3965,10 @@ rewrite_expr_tree (gimple *stmt, unsigned int opindex,
 	  update_stmt (stmt);
 	}
 
+      /* If the stmt that defines operand has to be inserted, insert it
+	 before the use after the use_stmt is inserted.  */
+      if (oe->stmt_to_insert)
+	insert_stmt_before_use (stmt, oe->stmt_to_insert);
       if (dump_file && (dump_flags & TDF_DETAILS))
 	{
 	  fprintf (dump_file, " into ");
@@ -4115,24 +4140,41 @@ rewrite_expr_tree_parallel (gassign *stmt, int width,
 	{
 	  /* If the stmt that defines operand has to be inserted, insert it
 	     before the use.  */
-	  if (stmt1)
-	    insert_stmt_before_use (stmts[i], stmt1);
-	  if (stmt2)
-	    insert_stmt_before_use (stmts[i], stmt2);
 	  gimple_assign_set_rhs1 (stmts[i], op1);
 	  gimple_assign_set_rhs2 (stmts[i], op2);
 	  update_stmt (stmts[i]);
 	}
       else
 	{
+	  /* PR71252: stmt_to_insert has to be inserted after use stmt created
+	     by build_and_add_sum. However if the other operand doesnt have define-stmt
+	     or is defined by GIMPLE_NOP, we have to insert it first.  */
+	  if (stmt1
+	      && (TREE_CODE (op2) != SSA_NAME
+		  || !gimple_bb (SSA_NAME_DEF_STMT (op2))
+		  || gimple_nop_p (SSA_NAME_DEF_STMT (op2))))
+	    {
+	      insert_stmt_before_use (stmts[i], stmt1);
+	      stmt1 = NULL;
+	    }
+	  if (stmt2
+	      && (TREE_CODE (op1) != SSA_NAME
+		  || !gimple_bb (SSA_NAME_DEF_STMT (op1))
+		  || gimple_nop_p (SSA_NAME_DEF_STMT (op1))))
+	    {
+	      insert_stmt_before_use (stmts[i], stmt2);
+	      stmt2 = NULL;
+	    }
 	  stmts[i] = build_and_add_sum (TREE_TYPE (last_rhs1), op1, op2, opcode);
-	  /* If the stmt that defines operand has to be inserted, insert it
-	     before new build_and_add stmt after it is created.  */
-	  if (stmt1)
-	    insert_stmt_before_use (stmts[i], stmt1);
-	  if (stmt2)
-	    insert_stmt_before_use (stmts[i], stmt2);
 	}
+
+      /* If the stmt that defines operand has to be inserted, insert it
+	 before new use stmt after it is created.  */
+      if (stmt1)
+	insert_stmt_before_use (stmts[i], stmt1);
+      if (stmt2)
+	insert_stmt_before_use (stmts[i], stmt2);
+      stmt1 = stmt2 = NULL;
       if (dump_file && (dump_flags & TDF_DETAILS))
 	{
 	  fprintf (dump_file, " into ");
@@ -5312,15 +5354,15 @@ reassociate_bb (basic_block bb)
 		{
 		  tree last_op = ops.last ()->op;
 
-		  /* If the stmt that defines operand has to be inserted, insert it
-		     before the use.  */
-		  if (ops.last ()->stmt_to_insert)
-		    insert_stmt_before_use (stmt, ops.last ()->stmt_to_insert);
 		  if (powi_result)
 		    transform_stmt_to_multiply (&gsi, stmt, last_op,
 						powi_result);
 		  else
 		    transform_stmt_to_copy (&gsi, stmt, last_op);
+		  /* If the stmt that defines operand has to be inserted, insert it
+		     before the use.  */
+		  if (ops.last ()->stmt_to_insert)
+		    insert_stmt_before_use (stmt, ops.last ()->stmt_to_insert);
 		}
 	      else
 		{