Message ID | 20191007162315.GM15914@tucnak |
---|---|
State | New |
Headers | show |
Series | [C++] Partial fix for further -fstrong-eval-order issues (PR c++/91987) | expand |
On 10/7/19 12:23 PM, Jakub Jelinek wrote: > 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 <jakub@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))) Perhaps we should handle this in cp_build_binary_op instead? > { > 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) > { Hmm, it's weird that we have both grok_array_decl and build_x_array_ref. I'll try removing the former. > --- 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); save_expr will make a copy of the array, won't it? That's unlikely to be what we want. Better to use cp_stabilize_reference, I'd think. In an ARRAY_REF the array operand is an lvalue operand, we don't need to guard against modification of the contents of the array in the index operand. > /* 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); I still think this pattern would be cleaner with a new gimple predicate that returns true for invariant || SSA_NAME. I think that would have the same effect as this code. > + } > + 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 >
On Mon, Oct 07, 2019 at 03:51:05PM -0400, Jason Merrill wrote: > > - 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))) > > Perhaps we should handle this in cp_build_binary_op instead? How? By adding a SAVE_EXPR in there, so that generic code is safe? > > if (processing_template_decl && expr != error_mark_node) > > { > > Hmm, it's weird that we have both grok_array_decl and build_x_array_ref. > I'll try removing the former. Indeed. I've noticed that because cp_build_array_ref only swaps in the non-array case, in: template <typename T, typename U> auto foo (T &x, U &y) { T t; U u; __builtin_memcpy (&t, &x, sizeof (t)); __builtin_memcpy (&u, &y, sizeof (u)); return t[u]; } int main () { int a[4] = { 3, 4, 5, 6 }; int b = 2; auto c = foo (a, b); auto d = foo (b, a); } we actually use the *(t+u) form in the second instantiation case (regardless of -fstrong-eval-order) and t[u] in the former case, as it is only grok_array_decl that swaps in that case. > > --- 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); > > save_expr will make a copy of the array, won't it? That's unlikely to be No. First of all, this is on the else branch of TREE_CODE (TREE_TYPE (array)) == ARRAY_TYPE, so either array is a pointer, or idx is an array, or pointer, and it is after cp_default_conversion, so I think ar must be either a pointer or integer. I haven't touched the ARRAY_REF case earlier, because that I believe is handled right in the gimplifier already. > > + 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); > > I still think this pattern would be cleaner with a new gimple predicate that > returns true for invariant || SSA_NAME. I think that would have the same > effect as this code. The problem is that we need a reliable way to discover safe GIMPLE temporaries for that to work. If SSA_NAME is created, great, but in various contexts (OpenMP/OpenACC bodies, and various other cases) allow_ssa is false, at which point the gimplifier creates a temporary artificial VAR_DECL. If there is a predicate that rejects such a temporary, gimplify_expr will ICE: gcc_assert (!VOID_TYPE_P (TREE_TYPE (*expr_p))); *expr_p = get_formal_tmp_var (*expr_p, pre_p); ... /* Make sure the temporary matches our predicate. */ gcc_assert ((*gimple_test_f) (*expr_p)); Now, we could have a predicate that does invariant || SSA_NAME || (VAR_P && DECL_ARTIFICIAL), but I'm not certain that is good enough, DECL_ARTIFICIAL are also TARGET_EXPR temporaries and many others and I couldn't convince myself one can't write a valid testcase that would still fail. Of course, we could add some VAR_DECL flag and set it on the create_tmp_from_val created temporaries, but is that the way to go? Jakub
On 10/7/19 4:10 PM, Jakub Jelinek wrote: > On Mon, Oct 07, 2019 at 03:51:05PM -0400, Jason Merrill wrote: >>> - 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))) >> >> Perhaps we should handle this in cp_build_binary_op instead? > > How? By adding a SAVE_EXPR in there, so that generic code is safe? Something like that, yes. >>> if (processing_template_decl && expr != error_mark_node) >>> { >> >> Hmm, it's weird that we have both grok_array_decl and build_x_array_ref. >> I'll try removing the former. > > Indeed. I've noticed that because cp_build_array_ref only swaps in the > non-array case, in: > template <typename T, typename U> > auto > foo (T &x, U &y) > { > T t; > U u; > __builtin_memcpy (&t, &x, sizeof (t)); > __builtin_memcpy (&u, &y, sizeof (u)); > return t[u]; > } > > int > main () > { > int a[4] = { 3, 4, 5, 6 }; > int b = 2; > auto c = foo (a, b); > auto d = foo (b, a); > } > we actually use the *(t+u) form in the second instantiation case > (regardless of -fstrong-eval-order) and t[u] in the former case, > as it is only grok_array_decl that swaps in that case. Aha. Yes, it seems there are a few things that work with grok_array_decl that will need to be fixed with build_x_array_ref. I'm not going to mess with this any more in stage 1. >>> --- 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); >> >> save_expr will make a copy of the array, won't it? That's unlikely to be > > No. First of all, this is on the else branch of > TREE_CODE (TREE_TYPE (array)) == ARRAY_TYPE, so either array is a pointer, > or idx is an array, or pointer, and it is after cp_default_conversion, so I > think ar must be either a pointer or integer. Ah, good point. > I haven't touched the ARRAY_REF case earlier, because that I believe is > handled right in the gimplifier already. >>> + 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); >> >> I still think this pattern would be cleaner with a new gimple predicate that >> returns true for invariant || SSA_NAME. I think that would have the same >> effect as this code. > > The problem is that we need a reliable way to discover safe GIMPLE > temporaries for that to work. If SSA_NAME is created, great, but in various > contexts (OpenMP/OpenACC bodies, and various other cases) allow_ssa is > false, at which point the gimplifier creates a temporary artificial VAR_DECL. Yes, but doesn't the code above have the exact same effect? You're checking for a variable that isn't an SSA_NAME, and making a copy otherwise. > If there is a predicate that rejects such a temporary, gimplify_expr will > ICE: > gcc_assert (!VOID_TYPE_P (TREE_TYPE (*expr_p))); > *expr_p = get_formal_tmp_var (*expr_p, pre_p); > ... > /* Make sure the temporary matches our predicate. */ > gcc_assert ((*gimple_test_f) (*expr_p)); Won't get_formal_tmp_var always give an SSA_NAME? It looks to me like it unconditionally passes true for allow_ssa. Jason
On Mon, Oct 07, 2019 at 04:37:13PM -0400, Jason Merrill wrote: > > How? By adding a SAVE_EXPR in there, so that generic code is safe? > > Something like that, yes. Ok, will try that tomorrow. > > I haven't touched the ARRAY_REF case earlier, because that I believe is > > handled right in the gimplifier already. > > > > + 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); > > > > > > I still think this pattern would be cleaner with a new gimple predicate that > > > returns true for invariant || SSA_NAME. I think that would have the same > > > effect as this code. > > > > The problem is that we need a reliable way to discover safe GIMPLE > > temporaries for that to work. If SSA_NAME is created, great, but in various > > contexts (OpenMP/OpenACC bodies, and various other cases) allow_ssa is > > false, at which point the gimplifier creates a temporary artificial VAR_DECL. > > Yes, but doesn't the code above have the exact same effect? You're checking > for a variable that isn't an SSA_NAME, and making a copy otherwise. It will have the same effect except for the ICE. > > If there is a predicate that rejects such a temporary, gimplify_expr will > > ICE: > > gcc_assert (!VOID_TYPE_P (TREE_TYPE (*expr_p))); > > *expr_p = get_formal_tmp_var (*expr_p, pre_p); > > ... > > /* Make sure the temporary matches our predicate. */ > > gcc_assert ((*gimple_test_f) (*expr_p)); > > Won't get_formal_tmp_var always give an SSA_NAME? It looks to me like it > unconditionally passes true for allow_ssa. Yes, but there is still the && gimplify_ctxp->into_ssa conditional. All but one push_gimplify_context call are with in_ssa = false. Jakub
On 10/7/19 4:57 PM, Jakub Jelinek wrote: > On Mon, Oct 07, 2019 at 04:37:13PM -0400, Jason Merrill wrote: >>> How? By adding a SAVE_EXPR in there, so that generic code is safe? >> >> Something like that, yes. > > Ok, will try that tomorrow. > >>> I haven't touched the ARRAY_REF case earlier, because that I believe is >>> handled right in the gimplifier already. >>>>> + 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); >>>> >>>> I still think this pattern would be cleaner with a new gimple predicate that >>>> returns true for invariant || SSA_NAME. I think that would have the same >>>> effect as this code. >>> >>> The problem is that we need a reliable way to discover safe GIMPLE >>> temporaries for that to work. If SSA_NAME is created, great, but in various >>> contexts (OpenMP/OpenACC bodies, and various other cases) allow_ssa is >>> false, at which point the gimplifier creates a temporary artificial VAR_DECL. >> >> Yes, but doesn't the code above have the exact same effect? You're checking >> for a variable that isn't an SSA_NAME, and making a copy otherwise. > > It will have the same effect except for the ICE. > >>> If there is a predicate that rejects such a temporary, gimplify_expr will >>> ICE: >>> gcc_assert (!VOID_TYPE_P (TREE_TYPE (*expr_p))); >>> *expr_p = get_formal_tmp_var (*expr_p, pre_p); >>> ... >>> /* Make sure the temporary matches our predicate. */ >>> gcc_assert ((*gimple_test_f) (*expr_p)); >> >> Won't get_formal_tmp_var always give an SSA_NAME? It looks to me like it >> unconditionally passes true for allow_ssa. > > Yes, but there is still the && gimplify_ctxp->into_ssa conditional. > All but one push_gimplify_context call are with in_ssa = false. 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? Jason
--- 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 (); +}