Hi!
So, C++17 and later says that in E1[E2] and E1 << E2 and E1 >> E2 the
E1 expression is sequenced before E2.
Similarly to the recent call expression PR, while the gimplifier
gimplifies the first operand before the second one, it uses is_gimple_val
as predicate (and we don't really have a usable predicate that would require
only gimple temporaries one can't modify), so if the first operand doesn't
gimplify into SSA_NAME which is ok, we need to force it into a temporary.
This is the cp-gimplify.c hunk. Unfortunately, for shifts that is not good
enough, because even before we reach that fold-const.c actually can move
some side-effects from the arguments, which is ok for the first operand, but
not ok for the second one.
For E1[E2], we either lower it into ARRAY_REF if E1 has array type, but in
that case I think the similar issue can't happen, or we lower it otherwise
to *(E1 + E2), but in that case there is absolutely no ordering guarantee
between the two, folders can swap them any time etc.
So, the patch instead for -fstrong-eval-order lowers it into
SAVE_EXPR<E1>, *(SAVE_EXPR<E1> + E2) if at least one of the operands has
side-effects. I had to also tweak grok_array_decl, because it swapped the
operands in some cases, and it causes on c-c++-common/gomp/reduction-1.c
I'll need to handle the new form of code in the OpenMP handling code.
Also, the PR contains other testcases that are wrong, but I'm not 100% sure
what to do, I'm afraid we'll need to force aggregate arguments (without
TREE_ADDRESSABLE types) into registers in that case for some of the
CALL_EXPRs with ordered args. Unlike this patch and the previous one, which
wastes at most some compile time memory for the extra temporaries or best
case SSA_NAMEs and one extra stmt to load), unlikely results in any runtime
slowdowns, forcing aggregates into separate temporaries might be very
expensive in some cases. So, wonder if we shouldn't do some smarter
analysis in that case, like if we have two args where the first one needs to
be evaluated before the second one and second one has side-effects (or vice
versa), for the first argument check if it has address taken or is global
(== may_be_aliased), if yes, probably force it always, otherwise walk the
second argument to see if it refers to this var. Thoughts on that?
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk
once I resolve the reduction-1.c ICE in the OpenMP code?
2019-10-07 Jakub Jelinek <ja...@redhat.com>
PR c++/91987
* fold-const.c (fold_binary_loc): Don't optimize COMPOUND_EXPR in
the second operand of -fstrong-eval-order shift/rotate.
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-gimplify.c (cp_gimplify_expr): For shifts/rotates and
-fstrong-eval-order, force the first operand into a temporary.
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.
--- gcc/fold-const.c.jj 2019-10-04 12:24:38.704764109 +0200
+++ gcc/fold-const.c 2019-10-07 13:21:56.855450821 +0200
@@ -9447,16 +9447,23 @@ fold_binary_loc (location_t loc, enum tr
if (TREE_CODE (arg0) == COMPOUND_EXPR)
{
tem = fold_build2_loc (loc, code, type,
- fold_convert_loc (loc, TREE_TYPE (op0),
- TREE_OPERAND (arg0, 1)), op1);
+ fold_convert_loc (loc, TREE_TYPE (op0),
+ TREE_OPERAND (arg0, 1)),
+ op1);
return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg0, 0),
tem);
}
- if (TREE_CODE (arg1) == COMPOUND_EXPR)
+ if (TREE_CODE (arg1) == COMPOUND_EXPR
+ && (flag_strong_eval_order != 2
+ /* C++17 disallows this canonicalization for shifts. */
+ || (code != LSHIFT_EXPR
+ && code != RSHIFT_EXPR
+ && code != LROTATE_EXPR
+ && code != RROTATE_EXPR)))
{
tem = fold_build2_loc (loc, code, type, op0,
- fold_convert_loc (loc, TREE_TYPE (op1),
- TREE_OPERAND (arg1, 1)));
+ fold_convert_loc (loc, TREE_TYPE (op1),
+ TREE_OPERAND (arg1, 1)));
return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg1, 0),
tem);
}
--- gcc/cp/decl2.c.jj 2019-09-26 21:34:21.711916155 +0200
+++ gcc/cp/decl2.c 2019-10-07 14:40:27.913111804 +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-07 13:08:58.717380894 +0200
+++ gcc/cp/typeck.c 2019-10-07 13:21:56.859450760 +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 = save_expr (ar);
/* Put the integer in IND to simplify error checking. */
if (TREE_CODE (TREE_TYPE (ar)) == INTEGER_TYPE)
@@ -3581,6 +3585,8 @@ cp_build_array_ref (location_t loc, tree
protected_set_expr_location (ret, loc);
if (non_lvalue)
ret = non_lvalue_loc (loc, ret);
+ if (first)
+ ret = build2_loc (loc, COMPOUND_EXPR, TREE_TYPE (ret), first, ret);
return ret;
}
}
--- gcc/cp/cp-gimplify.c.jj 2019-10-04 12:24:38.719763879 +0200
+++ gcc/cp/cp-gimplify.c 2019-10-07 13:21:56.856450806 +0200
@@ -884,6 +884,29 @@ cp_gimplify_expr (tree *expr_p, gimple_s
}
break;
+ case LSHIFT_EXPR:
+ case RSHIFT_EXPR:
+ case LROTATE_EXPR:
+ case RROTATE_EXPR:
+ /* If the second operand has side-effects, ensure the first operand
+ is evaluated first and is not a decl that the side-effects of the
+ second operand could modify. */
+ if (flag_strong_eval_order == 2
+ && TREE_SIDE_EFFECTS (TREE_OPERAND (*expr_p, 1)))
+ {
+ enum gimplify_status t
+ = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p,
+ is_gimple_val, fb_rvalue);
+ if (t == GS_ERROR)
+ break;
+ else if (is_gimple_variable (TREE_OPERAND (*expr_p, 0))
+ && TREE_CODE (TREE_OPERAND (*expr_p, 0)) != SSA_NAME)
+ TREE_OPERAND (*expr_p, 0)
+ = get_initialized_tmp_var (TREE_OPERAND (*expr_p, 0), pre_p,
+ NULL);
+ }
+ goto do_default;
+
case RETURN_EXPR:
if (TREE_OPERAND (*expr_p, 0)
&& (TREE_CODE (TREE_OPERAND (*expr_p, 0)) == INIT_EXPR
@@ -897,6 +920,7 @@ cp_gimplify_expr (tree *expr_p, gimple_s
}
/* Fall through. */
+ do_default:
default:
ret = (enum gimplify_status) c_gimplify_expr (expr_p, pre_p, post_p);
break;
--- gcc/testsuite/g++.dg/cpp1z/eval-order6.C.jj 2019-10-07 13:26:43.046053103
+0200
+++ gcc/testsuite/g++.dg/cpp1z/eval-order6.C 2019-10-07 13:28:30.202406497
+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-07 13:28:42.280220905
+0200
+++ gcc/testsuite/g++.dg/cpp1z/eval-order7.C 2019-10-07 13:29:18.207668835
+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-07 13:29:26.779537114
+0200
+++ gcc/testsuite/g++.dg/cpp1z/eval-order8.C 2019-10-07 13:30:06.644924526
+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 ();
+}
Jakub
