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

[Jason Merrill]

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