[C++] Partial fix for further -fstrong-eval-order issues (PR c++/91987, take 2)
diff mbox series

Message ID 20191010081328.GI15914@tucnak
State New
Headers show
Series
  • [C++] Partial fix for further -fstrong-eval-order issues (PR c++/91987, take 2)
Related show

Commit Message

Jakub Jelinek Oct. 10, 2019, 8:13 a.m. UTC
On Mon, Oct 07, 2019 at 05:17:56PM -0400, Jason Merrill wrote:
> Ah, I see.  And it occurs to me this situation fails condition #1 for
> get_formal_tmp_var anyway.  So I guess the predicate direction doesn't make
> sense.  Let's factor out the above pattern differently, then. Maybe a
> preevaluate function that takes a predicate as an argument?

Ok, so this patch does, compared to the previous one:
1) shifts are now done in cp_build_binary_op with the cp_save_expr,
   so no fold-const.c or cp-gimplify.c change is needed
2) the array ref x[y] -> *(x + y) where y has side-effects is now handled
   as *(SAVE_EXPR<x>, SAVE_EXPR<x> + y) rather than
   SAVE_EXPR<x>, *(SAVE_EXPR<x> + y) where the former is friendlier to
   wrapping it in ADDR_EXPR etc.; nevertheless, I had to disable it for the
   OpenMP reductions, I'll discuss in the language committee what to do if
   anything in that case
3) I've added the new function in cp-gimplify.c and used it in the CALL_EXPR
   case

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
If wanted, can commit the 3 unrelated changes separately.

And the call argument side-effects aren't handled yet.

2019-10-10  Jakub Jelinek  <jakub@redhat.com>

	PR c++/91987
cp/
	* decl2.c (grok_array_decl): For -fstrong-eval-order, when array ref
	operands have been swapped and at least one operand has side-effects,
	revert the swapping before calling build_array_ref.
	* typeck.c (cp_build_array_ref): For non-ARRAY_TYPE array ref with
	side-effects on the index operand, if -fstrong-eval-order use
	save_expr around the array operand.
	(cp_build_binary_op): For shifts with side-effects in the second
	operand, wrap first operand into SAVE_EXPR and evaluate it before
	the shift.
	* semantics.c (handle_omp_array_sections_1): Temporarily disable
	flag_strong_eval_order during OMP_CLAUSE_REDUCTION array section
	processing.
	* cp-gimplify.c (gimplify_to_rvalue): New function.
	(cp_gimplify_expr): Use it.
testsuite/
	* g++.dg/cpp1z/eval-order6.C: New test.
	* g++.dg/cpp1z/eval-order7.C: New test.
	* g++.dg/cpp1z/eval-order8.C: New test.
	* c-c++-common/gomp/pr91987.c: New test.



	Jakub

Comments

Jason Merrill Oct. 10, 2019, 5:54 p.m. UTC | #1
On 10/10/19 4:13 AM, Jakub Jelinek wrote:
> On Mon, Oct 07, 2019 at 05:17:56PM -0400, Jason Merrill wrote:
>> Ah, I see.  And it occurs to me this situation fails condition #1 for
>> get_formal_tmp_var anyway.  So I guess the predicate direction doesn't make
>> sense.  Let's factor out the above pattern differently, then. Maybe a
>> preevaluate function that takes a predicate as an argument?
> 
> Ok, so this patch does, compared to the previous one:
> 1) shifts are now done in cp_build_binary_op with the cp_save_expr,
>     so no fold-const.c or cp-gimplify.c change is needed
> 2) the array ref x[y] -> *(x + y) where y has side-effects is now handled
>     as *(SAVE_EXPR<x>, SAVE_EXPR<x> + y) rather than
>     SAVE_EXPR<x>, *(SAVE_EXPR<x> + y) where the former is friendlier to
>     wrapping it in ADDR_EXPR etc.; nevertheless, I had to disable it for the
>     OpenMP reductions, I'll discuss in the language committee what to do if
>     anything in that case
> 3) I've added the new function in cp-gimplify.c and used it in the CALL_EXPR
>     case
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> If wanted, can commit the 3 unrelated changes separately.
> 
> And the call argument side-effects aren't handled yet.
> 
> 2019-10-10  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/91987
> cp/
> 	* decl2.c (grok_array_decl): For -fstrong-eval-order, when array ref
> 	operands have been swapped and at least one operand has side-effects,
> 	revert the swapping before calling build_array_ref.
> 	* typeck.c (cp_build_array_ref): For non-ARRAY_TYPE array ref with
> 	side-effects on the index operand, if -fstrong-eval-order use
> 	save_expr around the array operand.
> 	(cp_build_binary_op): For shifts with side-effects in the second
> 	operand, wrap first operand into SAVE_EXPR and evaluate it before
> 	the shift.
> 	* semantics.c (handle_omp_array_sections_1): Temporarily disable
> 	flag_strong_eval_order during OMP_CLAUSE_REDUCTION array section
> 	processing.
> 	* cp-gimplify.c (gimplify_to_rvalue): New function.
> 	(cp_gimplify_expr): Use it.
> testsuite/
> 	* g++.dg/cpp1z/eval-order6.C: New test.
> 	* g++.dg/cpp1z/eval-order7.C: New test.
> 	* g++.dg/cpp1z/eval-order8.C: New test.
> 	* c-c++-common/gomp/pr91987.c: New test.
> 
> --- gcc/cp/decl2.c.jj	2019-10-09 10:27:12.704400889 +0200
> +++ gcc/cp/decl2.c	2019-10-09 10:32:44.932416373 +0200
> @@ -405,6 +405,7 @@ grok_array_decl (location_t loc, tree ar
>     else
>       {
>         tree p1, p2, i1, i2;
> +      bool swapped = false;
>   
>         /* Otherwise, create an ARRAY_REF for a pointer or array type.
>   	 It is a little-known fact that, if `a' is an array and `i' is
> @@ -431,7 +432,7 @@ grok_array_decl (location_t loc, tree ar
>         if (p1 && i2)
>   	array_expr = p1, index_exp = i2;
>         else if (i1 && p2)
> -	array_expr = p2, index_exp = i1;
> +	swapped = true, array_expr = p2, index_exp = i1;
>         else
>   	{
>   	  error_at (loc, "invalid types %<%T[%T]%> for array subscript",
> @@ -447,7 +448,12 @@ grok_array_decl (location_t loc, tree ar
>         else
>   	array_expr = mark_lvalue_use_nonread (array_expr);
>         index_exp = mark_rvalue_use (index_exp);
> -      expr = build_array_ref (input_location, array_expr, index_exp);
> +      if (swapped
> +	  && flag_strong_eval_order == 2
> +	  && (TREE_SIDE_EFFECTS (array_expr) || TREE_SIDE_EFFECTS (index_exp)))
> +	expr = build_array_ref (input_location, index_exp, array_expr);
> +      else
> +	expr = build_array_ref (input_location, array_expr, index_exp);
>       }
>     if (processing_template_decl && expr != error_mark_node)
>       {
> --- gcc/cp/typeck.c.jj	2019-10-09 10:27:12.625402077 +0200
> +++ gcc/cp/typeck.c	2019-10-09 11:21:09.058765959 +0200
> @@ -3550,6 +3550,10 @@ cp_build_array_ref (location_t loc, tree
>     {
>       tree ar = cp_default_conversion (array, complain);
>       tree ind = cp_default_conversion (idx, complain);
> +    tree first = NULL_TREE;
> +
> +    if (flag_strong_eval_order == 2 && TREE_SIDE_EFFECTS (ind))
> +      ar = first = cp_save_expr (ar);
>   
>       /* Put the integer in IND to simplify error checking.  */
>       if (TREE_CODE (TREE_TYPE (ar)) == INTEGER_TYPE)
> @@ -3573,11 +3577,10 @@ cp_build_array_ref (location_t loc, tree
>   
>       warn_array_subscript_with_type_char (loc, idx);
>   
> -    ret = cp_build_indirect_ref (cp_build_binary_op (input_location,
> -						     PLUS_EXPR, ar, ind,
> -						     complain),
> -                                 RO_ARRAY_INDEXING,
> -                                 complain);
> +    ret = cp_build_binary_op (input_location, PLUS_EXPR, ar, ind, complain);
> +    if (first)
> +      ret = build2_loc (loc, COMPOUND_EXPR, TREE_TYPE (ret), first, ret);
> +    ret = cp_build_indirect_ref (ret, RO_ARRAY_INDEXING, complain);
>       protected_set_expr_location (ret, loc);
>       if (non_lvalue)
>         ret = non_lvalue_loc (loc, ret);
> @@ -5555,6 +5558,17 @@ cp_build_binary_op (const op_location_t
>     if (build_type == NULL_TREE)
>       build_type = result_type;
>   
> +  if (doing_shift
> +      && flag_strong_eval_order == 2
> +      && TREE_SIDE_EFFECTS (op1)
> +      && !processing_template_decl)
> +    {
> +      /* In C++17, in both op0 << op1 and op0 >> op1 op0 is sequenced before
> +	 op1, so if op1 has side-effects, use SAVE_EXPR around op0.  */
> +      op0 = cp_save_expr (op0);
> +      instrument_expr = op0;
> +    }
> +
>     if (sanitize_flags_p ((SANITIZE_SHIFT
>   			 | SANITIZE_DIVIDE | SANITIZE_FLOAT_DIVIDE))
>         && current_function_decl != NULL_TREE
> @@ -5566,6 +5580,7 @@ cp_build_binary_op (const op_location_t
>         op1 = cp_save_expr (op1);
>         op0 = fold_non_dependent_expr (op0, complain);
>         op1 = fold_non_dependent_expr (op1, complain);
> +      tree instrument_expr1 = NULL_TREE;
>         if (doing_div_or_mod
>   	  && sanitize_flags_p (SANITIZE_DIVIDE | SANITIZE_FLOAT_DIVIDE))
>   	{
> @@ -5578,10 +5593,15 @@ cp_build_binary_op (const op_location_t
>   	    cop0 = cp_convert (orig_type, op0, complain);
>   	  if (TREE_TYPE (cop1) != orig_type)
>   	    cop1 = cp_convert (orig_type, op1, complain);
> -	  instrument_expr = ubsan_instrument_division (location, cop0, cop1);
> +	  instrument_expr1 = ubsan_instrument_division (location, cop0, cop1);
>   	}
>         else if (doing_shift && sanitize_flags_p (SANITIZE_SHIFT))
> -	instrument_expr = ubsan_instrument_shift (location, code, op0, op1);
> +	instrument_expr1 = ubsan_instrument_shift (location, code, op0, op1);
> +      if (instrument_expr != NULL)
> +	instrument_expr = build2 (COMPOUND_EXPR, TREE_TYPE (instrument_expr1),
> +				  instrument_expr, instrument_expr1);
> +      else
> +	instrument_expr = instrument_expr1;

You could use add_stmt_to_compound here.  OK either way.

Jason

Patch
diff mbox series

--- gcc/cp/decl2.c.jj	2019-10-09 10:27:12.704400889 +0200
+++ gcc/cp/decl2.c	2019-10-09 10:32:44.932416373 +0200
@@ -405,6 +405,7 @@  grok_array_decl (location_t loc, tree ar
   else
     {
       tree p1, p2, i1, i2;
+      bool swapped = false;
 
       /* Otherwise, create an ARRAY_REF for a pointer or array type.
 	 It is a little-known fact that, if `a' is an array and `i' is
@@ -431,7 +432,7 @@  grok_array_decl (location_t loc, tree ar
       if (p1 && i2)
 	array_expr = p1, index_exp = i2;
       else if (i1 && p2)
-	array_expr = p2, index_exp = i1;
+	swapped = true, array_expr = p2, index_exp = i1;
       else
 	{
 	  error_at (loc, "invalid types %<%T[%T]%> for array subscript",
@@ -447,7 +448,12 @@  grok_array_decl (location_t loc, tree ar
       else
 	array_expr = mark_lvalue_use_nonread (array_expr);
       index_exp = mark_rvalue_use (index_exp);
-      expr = build_array_ref (input_location, array_expr, index_exp);
+      if (swapped
+	  && flag_strong_eval_order == 2
+	  && (TREE_SIDE_EFFECTS (array_expr) || TREE_SIDE_EFFECTS (index_exp)))
+	expr = build_array_ref (input_location, index_exp, array_expr);
+      else
+	expr = build_array_ref (input_location, array_expr, index_exp);
     }
   if (processing_template_decl && expr != error_mark_node)
     {
--- gcc/cp/typeck.c.jj	2019-10-09 10:27:12.625402077 +0200
+++ gcc/cp/typeck.c	2019-10-09 11:21:09.058765959 +0200
@@ -3550,6 +3550,10 @@  cp_build_array_ref (location_t loc, tree
   {
     tree ar = cp_default_conversion (array, complain);
     tree ind = cp_default_conversion (idx, complain);
+    tree first = NULL_TREE;
+
+    if (flag_strong_eval_order == 2 && TREE_SIDE_EFFECTS (ind))
+      ar = first = cp_save_expr (ar);
 
     /* Put the integer in IND to simplify error checking.  */
     if (TREE_CODE (TREE_TYPE (ar)) == INTEGER_TYPE)
@@ -3573,11 +3577,10 @@  cp_build_array_ref (location_t loc, tree
 
     warn_array_subscript_with_type_char (loc, idx);
 
-    ret = cp_build_indirect_ref (cp_build_binary_op (input_location,
-						     PLUS_EXPR, ar, ind,
-						     complain),
-                                 RO_ARRAY_INDEXING,
-                                 complain);
+    ret = cp_build_binary_op (input_location, PLUS_EXPR, ar, ind, complain);
+    if (first)
+      ret = build2_loc (loc, COMPOUND_EXPR, TREE_TYPE (ret), first, ret);
+    ret = cp_build_indirect_ref (ret, RO_ARRAY_INDEXING, complain);
     protected_set_expr_location (ret, loc);
     if (non_lvalue)
       ret = non_lvalue_loc (loc, ret);
@@ -5555,6 +5558,17 @@  cp_build_binary_op (const op_location_t
   if (build_type == NULL_TREE)
     build_type = result_type;
 
+  if (doing_shift
+      && flag_strong_eval_order == 2
+      && TREE_SIDE_EFFECTS (op1)
+      && !processing_template_decl)
+    {
+      /* In C++17, in both op0 << op1 and op0 >> op1 op0 is sequenced before
+	 op1, so if op1 has side-effects, use SAVE_EXPR around op0.  */
+      op0 = cp_save_expr (op0);
+      instrument_expr = op0;
+    }
+
   if (sanitize_flags_p ((SANITIZE_SHIFT
 			 | SANITIZE_DIVIDE | SANITIZE_FLOAT_DIVIDE))
       && current_function_decl != NULL_TREE
@@ -5566,6 +5580,7 @@  cp_build_binary_op (const op_location_t
       op1 = cp_save_expr (op1);
       op0 = fold_non_dependent_expr (op0, complain);
       op1 = fold_non_dependent_expr (op1, complain);
+      tree instrument_expr1 = NULL_TREE;
       if (doing_div_or_mod
 	  && sanitize_flags_p (SANITIZE_DIVIDE | SANITIZE_FLOAT_DIVIDE))
 	{
@@ -5578,10 +5593,15 @@  cp_build_binary_op (const op_location_t
 	    cop0 = cp_convert (orig_type, op0, complain);
 	  if (TREE_TYPE (cop1) != orig_type)
 	    cop1 = cp_convert (orig_type, op1, complain);
-	  instrument_expr = ubsan_instrument_division (location, cop0, cop1);
+	  instrument_expr1 = ubsan_instrument_division (location, cop0, cop1);
 	}
       else if (doing_shift && sanitize_flags_p (SANITIZE_SHIFT))
-	instrument_expr = ubsan_instrument_shift (location, code, op0, op1);
+	instrument_expr1 = ubsan_instrument_shift (location, code, op0, op1);
+      if (instrument_expr != NULL)
+	instrument_expr = build2 (COMPOUND_EXPR, TREE_TYPE (instrument_expr1),
+				  instrument_expr, instrument_expr1);
+      else
+	instrument_expr = instrument_expr1;
     }
 
   result = build2_loc (location, resultcode, build_type, op0, op1);
--- gcc/cp/semantics.c.jj	2019-09-27 22:13:18.336903781 +0200
+++ gcc/cp/semantics.c	2019-10-09 12:25:46.852235717 +0200
@@ -5017,6 +5017,15 @@  handle_omp_array_sections_1 (tree c, tre
       TREE_PURPOSE (t) = lb;
       low_bound = lb;
     }
+  /* Temporarily disable -fstrong-eval-order for array reductions.
+     The SAVE_EXPR and COMPOUND_EXPR added if low_bound has side-effects
+     is something the middle-end can't cope with and more importantly,
+     it needs to be the actual base variable that is privatized, not some
+     temporary assigned previous value of it.  That, together with OpenMP
+     saying how many times the side-effects are evaluated is unspecified,
+     makes int *a, *b; ... reduction(+:a[a = b, 3:10]) really unspecified.  */
+  warning_sentinel s (flag_strong_eval_order,
+		      OMP_CLAUSE_CODE (c) == OMP_CLAUSE_REDUCTION);
   ret = grok_array_decl (OMP_CLAUSE_LOCATION (c), ret, low_bound, false);
   return ret;
 }
--- gcc/cp/cp-gimplify.c.jj	2019-10-09 10:27:12.654401641 +0200
+++ gcc/cp/cp-gimplify.c	2019-10-09 10:32:44.930416402 +0200
@@ -638,6 +638,22 @@  lvalue_has_side_effects (tree e)
     return TREE_SIDE_EFFECTS (e);
 }
 
+/* Gimplify *EXPR_P as rvalue into an expression that can't be modified
+   by expressions with side-effects in other operands.  */
+
+static enum gimplify_status
+gimplify_to_rvalue (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
+		    bool (*gimple_test_f) (tree))
+{
+  enum gimplify_status t
+    = gimplify_expr (expr_p, pre_p, post_p, gimple_test_f, fb_rvalue);
+  if (t == GS_ERROR)
+    return GS_ERROR;
+  else if (is_gimple_variable (*expr_p) && TREE_CODE (*expr_p) != SSA_NAME)
+    *expr_p = get_initialized_tmp_var (*expr_p, pre_p, NULL);
+  return t;
+}
+
 /* Do C++-specific gimplification.  Args are as for gimplify_expr.  */
 
 int
@@ -823,15 +839,10 @@  cp_gimplify_expr (tree *expr_p, gimple_s
 	  && cp_get_callee_fndecl_nofold (*expr_p) == NULL_TREE)
 	{
 	  enum gimplify_status t
-	    = gimplify_expr (&CALL_EXPR_FN (*expr_p), pre_p, NULL,
-			     is_gimple_call_addr, fb_rvalue);
+	    = gimplify_to_rvalue (&CALL_EXPR_FN (*expr_p), pre_p, NULL,
+				  is_gimple_call_addr);
 	  if (t == GS_ERROR)
 	    ret = GS_ERROR;
-	  else if (is_gimple_variable (CALL_EXPR_FN (*expr_p))
-		   && TREE_CODE (CALL_EXPR_FN (*expr_p)) != SSA_NAME)
-	    CALL_EXPR_FN (*expr_p)
-	      = get_initialized_tmp_var (CALL_EXPR_FN (*expr_p), pre_p,
-					 NULL);
 	}
       if (!CALL_EXPR_FN (*expr_p))
 	/* Internal function call.  */;
--- gcc/testsuite/g++.dg/cpp1z/eval-order6.C.jj	2019-10-09 10:32:44.935416328 +0200
+++ gcc/testsuite/g++.dg/cpp1z/eval-order6.C	2019-10-09 10:32:44.935416328 +0200
@@ -0,0 +1,20 @@ 
+// PR c++/91987
+// { dg-do run }
+// { dg-options "-fstrong-eval-order" }
+
+int
+foo ()
+{
+  int x = 5;
+  int r = x << (x = 3, 2);
+  if (x != 3)
+    __builtin_abort ();
+  return r;
+}
+
+int
+main ()
+{
+  if (foo () != (5 << 2))
+    __builtin_abort ();
+}
--- gcc/testsuite/g++.dg/cpp1z/eval-order7.C.jj	2019-10-09 10:32:44.934416343 +0200
+++ gcc/testsuite/g++.dg/cpp1z/eval-order7.C	2019-10-09 10:32:44.934416343 +0200
@@ -0,0 +1,23 @@ 
+// PR c++/91987
+// { dg-do run }
+// { dg-options "-fstrong-eval-order" }
+
+int a[4] = { 1, 2, 3, 4 };
+int b[4] = { 5, 6, 7, 8 };
+
+int
+foo ()
+{
+  int *x = a;
+  int r = x[(x = b, 3)];
+  if (x != b)
+    __builtin_abort ();
+  return r;
+}
+
+int
+main ()
+{
+  if (foo () != 4)
+    __builtin_abort ();
+}
--- gcc/testsuite/g++.dg/cpp1z/eval-order8.C.jj	2019-10-09 10:32:44.935416328 +0200
+++ gcc/testsuite/g++.dg/cpp1z/eval-order8.C	2019-10-09 10:32:44.935416328 +0200
@@ -0,0 +1,20 @@ 
+// PR c++/91987
+// { dg-do run }
+// { dg-options "-fstrong-eval-order" }
+
+int a[4] = { 1, 2, 3, 4 };
+int b;
+
+int
+main ()
+{
+  int *x = a;
+  b = 1;
+  int r = (b = 4, x)[(b *= 2, 3)];
+  if (b != 8 || r != 4)
+    __builtin_abort ();
+  b = 1;
+  r = (b = 3, 2)[(b *= 2, x)];
+  if (b != 6 || r != 3)
+    __builtin_abort ();
+}
--- gcc/testsuite/c-c++-common/gomp/pr91987.c.jj	2019-10-09 12:42:40.059990464 +0200
+++ gcc/testsuite/c-c++-common/gomp/pr91987.c	2019-10-09 12:42:51.330820905 +0200
@@ -0,0 +1,26 @@ 
+/* PR c++/91987 */
+
+int bar (void);
+void baz (int *);
+#pragma omp declare target to (baz)
+
+void
+foo (int *a, int (*b)[10][10])
+{
+  #pragma omp target map(a[bar ()])
+  baz (a);
+  #pragma omp target map(a[bar ():1])
+  baz (a);
+  #pragma omp target map(a[10:bar ()])
+  baz (a);
+  #pragma omp task depend(inout:a[10:bar ()])
+  baz (a);
+  #pragma omp task depend(inout:a[10:bar ()])
+  baz (a);
+  #pragma omp parallel reduction(+:a[bar ():2])
+  baz (a);
+  #pragma omp parallel reduction(+:a[2:bar ()])
+  baz (a);
+  #pragma omp parallel reduction(+:b[bar ():2][bar ():10][bar ():10])
+  baz (a);
+}