diff mbox series

[C++] Partial fix for further -fstrong-eval-order issues (PR c++/91987)

Message ID 20191007162315.GM15914@tucnak
State New
Headers show
Series [C++] Partial fix for further -fstrong-eval-order issues (PR c++/91987) | expand

Commit Message

Jakub Jelinek Oct. 7, 2019, 4:23 p.m. UTC 
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.


	Jakub

Comments

Jason Merrill Oct. 7, 2019, 7:51 p.m. UTC | #1 
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
>
Jakub Jelinek Oct. 7, 2019, 8:10 p.m. UTC | #2 
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
Jason Merrill Oct. 7, 2019, 8:37 p.m. UTC | #3 
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
Jakub Jelinek Oct. 7, 2019, 8:57 p.m. UTC | #4 
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
Jason Merrill Oct. 7, 2019, 9:17 p.m. UTC | #5 
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
diff mbox series

Patch

--- 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 ();
+}