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
