[C PATCH] Preevaluate rhs for lhs op= rhs in C (PR c/58943)

[C PATCH] Preevaluate rhs for lhs op= rhs in C (PR c/58943)

[Jakub Jelinek]

Hi!

This patch fixes the following testcase by preevaluating rhs if it has
(can have) side-effects in lhs op= rhs expressions.  Bootstrapped/regtested
on x86_64-linux and i686-linux, ok for trunk?
C++ already does a similar thing (though in that case with TARGET_EXPRs).

Note1: had to tweak ssa-fre-33.c testcase a little bit (but it still fails
without the fix which went together with it and succeeds with the fix
and from that point onwards), because before fre1 there isn't enough forward
propagation that would make it constant (the addition result becomes
constant during fre1).

Note2: c-c++-common/cilk-plus/AN/rank_mismatch2.c ICEs now, supposedly
array notation handling doesn't handle SAVE_EXPRs properly, Balaji, do you
think you can debug it and fix up afterwards?

2014-01-13  Jakub Jelinek  <jakub@redhat.com>

	PR c/58943
	* c-typeck.c (build_modify_expr): For lhs op= rhs, if rhs has side
	effects, preevaluate rhs using SAVE_EXPR first.

	* c-omp.c (c_finish_omp_atomic): Set in_late_binary_op around
	build_modify_expr with non-NOP_EXPR opcode.  Handle return from it
	being COMPOUND_EXPR.
	(c_finish_omp_for): Handle incr being COMPOUND_EXPR with first
	operand a SAVE_EXPR and second MODIFY_EXPR.

	* gcc.c-torture/execute/pr58943.c: New test.
	* gcc.dg/tree-ssa/ssa-fre-33.c (main): Avoid using += in the test.

--- gcc/c/c-typeck.c.jj	2014-01-04 09:48:20.845147744 +0100
+++ gcc/c/c-typeck.c	2014-01-13 14:57:27.133743740 +0100
@@ -5193,6 +5193,7 @@ build_modify_expr (location_t location,
 {
   tree result;
   tree newrhs;
+  tree rhseval = NULL_TREE;
   tree rhs_semantic_type = NULL_TREE;
   tree lhstype = TREE_TYPE (lhs);
   tree olhstype = lhstype;
@@ -5254,8 +5255,17 @@ build_modify_expr (location_t location,
       /* Construct the RHS for any non-atomic compound assignemnt. */
       if (!is_atomic_op)
         {
+	  /* If in LHS op= RHS the RHS has side-effects, ensure they
+	     are preevaluated before the rest of the assignment expression's
+	     side-effects, because RHS could contain e.g. function calls
+	     that modify LHS.  */
+	  if (TREE_SIDE_EFFECTS (rhs))
+	    {
+	      newrhs = in_late_binary_op ? save_expr (rhs) : c_save_expr (rhs);
+	      rhseval = newrhs;
+	    }
 	  newrhs = build_binary_op (location,
-				    modifycode, lhs, rhs, 1);
+				    modifycode, lhs, newrhs, 1);
 
 	  /* The original type of the right hand side is no longer
 	     meaningful.  */
@@ -5269,7 +5279,7 @@ build_modify_expr (location_t location,
 	 if so, we need to generate setter calls.  */
       result = objc_maybe_build_modify_expr (lhs, newrhs);
       if (result)
-	return result;
+	goto return_result;
 
       /* Else, do the check that we postponed for Objective-C.  */
       if (!lvalue_or_else (location, lhs, lv_assign))
@@ -5363,7 +5373,7 @@ build_modify_expr (location_t location,
       if (result)
 	{
 	  protected_set_expr_location (result, location);
-	  return result;
+	  goto return_result;
 	}
     }
 
@@ -5384,11 +5394,15 @@ build_modify_expr (location_t location,
      as the LHS argument.  */
 
   if (olhstype == TREE_TYPE (result))
-    return result;
+    goto return_result;
 
   result = convert_for_assignment (location, olhstype, result, rhs_origtype,
 				   ic_assign, false, NULL_TREE, NULL_TREE, 0);
   protected_set_expr_location (result, location);
+
+return_result:
+  if (rhseval)
+    result = build2 (COMPOUND_EXPR, TREE_TYPE (result), rhseval, result);
   return result;
 }
 \f
--- gcc/c-family/c-omp.c.jj	2014-01-04 09:48:20.000000000 +0100
+++ gcc/c-family/c-omp.c	2014-01-13 15:23:51.653591098 +0100
@@ -136,7 +136,7 @@ c_finish_omp_atomic (location_t loc, enu
 		     enum tree_code opcode, tree lhs, tree rhs,
 		     tree v, tree lhs1, tree rhs1, bool swapped, bool seq_cst)
 {
-  tree x, type, addr;
+  tree x, type, addr, pre = NULL_TREE;
 
   if (lhs == error_mark_node || rhs == error_mark_node
       || v == error_mark_node || lhs1 == error_mark_node
@@ -194,9 +194,18 @@ c_finish_omp_atomic (location_t loc, enu
       rhs = build2_loc (loc, opcode, TREE_TYPE (lhs), rhs, lhs);
       opcode = NOP_EXPR;
     }
+  bool save = in_late_binary_op;
+  in_late_binary_op = true;
   x = build_modify_expr (loc, lhs, NULL_TREE, opcode, loc, rhs, NULL_TREE);
+  in_late_binary_op = save;
   if (x == error_mark_node)
     return error_mark_node;
+  if (TREE_CODE (x) == COMPOUND_EXPR)
+    {
+      pre = TREE_OPERAND (x, 0);
+      gcc_assert (TREE_CODE (pre) == SAVE_EXPR);
+      x = TREE_OPERAND (x, 1);
+    }
   gcc_assert (TREE_CODE (x) == MODIFY_EXPR);
   rhs = TREE_OPERAND (x, 1);
 
@@ -264,6 +273,8 @@ c_finish_omp_atomic (location_t loc, enu
       x = omit_one_operand_loc (loc, type, x, rhs1addr);
     }
 
+  if (pre)
+    x = omit_one_operand_loc (loc, type, x, pre);
   return x;
 }
 
@@ -555,6 +566,12 @@ c_finish_omp_for (location_t locus, enum
 	      incr = c_omp_for_incr_canonicalize_ptr (elocus, decl, incr);
 	      break;
 
+	    case COMPOUND_EXPR:
+	      if (TREE_CODE (TREE_OPERAND (incr, 0)) != SAVE_EXPR
+		  || TREE_CODE (TREE_OPERAND (incr, 1)) != MODIFY_EXPR)
+		break;
+	      incr = TREE_OPERAND (incr, 1);
+	      /* FALLTHRU */
 	    case MODIFY_EXPR:
 	      if (TREE_OPERAND (incr, 0) != decl)
 		break;
--- gcc/testsuite/gcc.c-torture/execute/pr58943.c.jj1	2014-01-13 15:43:39.391463496 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr58943.c	2014-01-13 15:43:32.591490400 +0100
@@ -0,0 +1,19 @@
+/* PR c/58943 */
+
+unsigned int x[1] = { 2 };
+
+unsigned int
+foo (void)
+{
+  x[0] |= 128;
+  return 1;
+}
+
+int
+main ()
+{
+  x[0] |= foo ();
+  if (x[0] != 131)
+    __builtin_abort ();
+  return 0;
+}
--- gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-33.c.jj	2012-02-09 12:11:45.000000000 +0100
+++ gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-33.c	2014-01-13 19:13:20.000000000 +0100
@@ -11,8 +11,8 @@ struct {
 float x;
 int main(int argc)
 {
-  vector float res = (vector float){0.0f,0.0f,0.0f,1.0f};
-  res += (vector float){1.0f,2.0f,3.0f,4.0f};
+  vector float res;
+  res = (vector float){1.0f,2.0f,3.0f,5.0f};
   s.global_res = res;
   x = *((float*)&s.global_res + 1);
   return 0;

	Jakub

Re: [C PATCH] Preevaluate rhs for lhs op= rhs in C (PR c/58943)

[Joseph S. Myers]

On Mon, 13 Jan 2014, Jakub Jelinek wrote:

> This patch fixes the following testcase by preevaluating rhs if it has
> (can have) side-effects in lhs op= rhs expressions.  Bootstrapped/regtested
> on x86_64-linux and i686-linux, ok for trunk?

OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [C PATCH] Preevaluate rhs for lhs op= rhs in C (PR c/58943)

[H.J. Lu]

On Mon, Jan 13, 2014 at 11:11 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> This patch fixes the following testcase by preevaluating rhs if it has
> (can have) side-effects in lhs op= rhs expressions.  Bootstrapped/regtested
> on x86_64-linux and i686-linux, ok for trunk?
> C++ already does a similar thing (though in that case with TARGET_EXPRs).
>
> Note1: had to tweak ssa-fre-33.c testcase a little bit (but it still fails
> without the fix which went together with it and succeeds with the fix
> and from that point onwards), because before fre1 there isn't enough forward
> propagation that would make it constant (the addition result becomes
> constant during fre1).
>
> Note2: c-c++-common/cilk-plus/AN/rank_mismatch2.c ICEs now, supposedly
> array notation handling doesn't handle SAVE_EXPRs properly, Balaji, do you
> think you can debug it and fix up afterwards?
>
> 2014-01-13  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c/58943
>         * c-typeck.c (build_modify_expr): For lhs op= rhs, if rhs has side
>         effects, preevaluate rhs using SAVE_EXPR first.
>
>         * c-omp.c (c_finish_omp_atomic): Set in_late_binary_op around
>         build_modify_expr with non-NOP_EXPR opcode.  Handle return from it
>         being COMPOUND_EXPR.
>         (c_finish_omp_for): Handle incr being COMPOUND_EXPR with first
>         operand a SAVE_EXPR and second MODIFY_EXPR.
>

This caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59825

-- 
H.J.