c++, v2: Fix up -fstrong-eval-order handling of call arguments [PR70796]
Commit Message
On Thu, Nov 04, 2021 at 03:07:57PM +0100, Jakub Jelinek via Gcc-patches wrote:
> For the METHOD_TYPE first argument
> I use a temporary always though, that should be always is_gimple_reg_type...
Doing so regressed
+FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++11 scan-tree-dump gimple "V::V .this, _1.;"
+FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++11 scan-tree-dump gimple "Y::Y ._2, _3.;"
+FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++14 scan-tree-dump gimple "V::V .this, _1.;"
+FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++14 scan-tree-dump gimple "Y::Y ._2, _3.;"
+FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++17 scan-tree-dump gimple "V::V .this, _1.;"
+FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++17 scan-tree-dump gimple "Y::Y ._2, _3.;"
+FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++20 scan-tree-dump gimple "V::V .this, _1.;"
+FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++20 scan-tree-dump gimple "Y::Y ._2, _3.;"
+FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++2b scan-tree-dump gimple "V::V .this, _1.;"
+FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++2b scan-tree-dump gimple "Y::Y ._2, _3.;"
because the testcase relies on this being passed directly in gimple dump,
rather than some SSA_NAME based on this.
Instead of changing the testcase, I've figured out that it is actually quite
easy to restore previous behavior here, for 2 reasons even.
One is that there are no side-effects in the ctor call arguments, so
the forcing of this into a temporary wasn't really needed, we can like in
the other cases quite cheaply see if the call has any side-effect arguments.
And the other reason is that in C++ this can't be modified, and similarly
vars with reference type can't be modified, so for those we don't need to
force them into a temporary either even if there are side-effects.
This means e.g. on
struct S
{
void foo (S &, int);
void bar (int);
};
void S::foo (S &p, int x)
{
this->bar (++x);
p.bar (++x);
}
we can keep what we were emitting before even for -std=c++17.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk
and after a while for 7.3 too?
2021-11-05 Jakub Jelinek <jakub@redhat.com>
PR c++/70796
* cp-gimplify.c (cp_gimplify_arg): New function.
(cp_gimplify_expr): Use cp_gimplify_arg instead of gimplify_arg,
pass true as last argument to it if there are any following
arguments in strong evaluation order with side-effects.
* g++.dg/cpp1z/eval-order11.C: New test.
Jakub
Comments
On 11/5/21 06:07, Jakub Jelinek wrote:
> On Thu, Nov 04, 2021 at 03:07:57PM +0100, Jakub Jelinek via Gcc-patches wrote:
>> For the METHOD_TYPE first argument
>> I use a temporary always though, that should be always is_gimple_reg_type...
>
> Doing so regressed
> +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++11 scan-tree-dump gimple "V::V .this, _1.;"
> +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++11 scan-tree-dump gimple "Y::Y ._2, _3.;"
> +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++14 scan-tree-dump gimple "V::V .this, _1.;"
> +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++14 scan-tree-dump gimple "Y::Y ._2, _3.;"
> +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++17 scan-tree-dump gimple "V::V .this, _1.;"
> +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++17 scan-tree-dump gimple "Y::Y ._2, _3.;"
> +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++20 scan-tree-dump gimple "V::V .this, _1.;"
> +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++20 scan-tree-dump gimple "Y::Y ._2, _3.;"
> +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++2b scan-tree-dump gimple "V::V .this, _1.;"
> +FAIL: g++.dg/cpp1z/inh-ctor23.C -std=gnu++2b scan-tree-dump gimple "Y::Y ._2, _3.;"
> because the testcase relies on this being passed directly in gimple dump,
> rather than some SSA_NAME based on this.
> Instead of changing the testcase, I've figured out that it is actually quite
> easy to restore previous behavior here, for 2 reasons even.
> One is that there are no side-effects in the ctor call arguments, so
> the forcing of this into a temporary wasn't really needed, we can like in
> the other cases quite cheaply see if the call has any side-effect arguments.
> And the other reason is that in C++ this can't be modified, and similarly
> vars with reference type can't be modified, so for those we don't need to
> force them into a temporary either even if there are side-effects.
> This means e.g. on
> struct S
> {
> void foo (S &, int);
> void bar (int);
> };
>
> void S::foo (S &p, int x)
> {
> this->bar (++x);
> p.bar (++x);
> }
> we can keep what we were emitting before even for -std=c++17.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk
> and after a while for 7.3 too?
OK.
> 2021-11-05 Jakub Jelinek <jakub@redhat.com>
>
> PR c++/70796
> * cp-gimplify.c (cp_gimplify_arg): New function.
> (cp_gimplify_expr): Use cp_gimplify_arg instead of gimplify_arg,
> pass true as last argument to it if there are any following
> arguments in strong evaluation order with side-effects.
>
> * g++.dg/cpp1z/eval-order11.C: New test.
>
> --- gcc/cp/cp-gimplify.c.jj 2021-10-29 19:33:10.542344939 +0200
> +++ gcc/cp/cp-gimplify.c 2021-11-05 00:41:29.124227336 +0100
> @@ -398,6 +398,47 @@ gimplify_to_rvalue (tree *expr_p, gimple
> return t;
> }
>
> +/* Like gimplify_arg, but if ORDERED is set (which should be set if
> + any of the arguments this argument is sequenced before has
> + TREE_SIDE_EFFECTS set, make sure expressions with is_gimple_reg_type type
> + are gimplified into SSA_NAME or a fresh temporary and for
> + non-is_gimple_reg_type we don't optimize away TARGET_EXPRs. */
> +
> +static enum gimplify_status
> +cp_gimplify_arg (tree *arg_p, gimple_seq *pre_p, location_t call_location,
> + bool ordered)
> +{
> + enum gimplify_status t;
> + if (ordered
> + && !is_gimple_reg_type (TREE_TYPE (*arg_p))
> + && TREE_CODE (*arg_p) == TARGET_EXPR)
> + {
> + /* gimplify_arg would strip away the TARGET_EXPR, but
> + that can mean we don't copy the argument and some following
> + argument with side-effect could modify it. */
> + protected_set_expr_location (*arg_p, call_location);
> + return gimplify_expr (arg_p, pre_p, NULL, is_gimple_lvalue, fb_either);
> + }
> + else
> + {
> + t = gimplify_arg (arg_p, pre_p, call_location);
> + if (t == GS_ERROR)
> + return GS_ERROR;
> + else if (ordered
> + && is_gimple_reg_type (TREE_TYPE (*arg_p))
> + && is_gimple_variable (*arg_p)
> + && TREE_CODE (*arg_p) != SSA_NAME
> + /* No need to force references into register, references
> + can't be modified. */
> + && !TYPE_REF_P (TREE_TYPE (*arg_p))
> + /* And this can't be modified either. */
> + && *arg_p != current_class_ptr)
> + *arg_p = get_initialized_tmp_var (*arg_p, pre_p);
> + return t;
> + }
> +
> +}
> +
> /* Do C++-specific gimplification. Args are as for gimplify_expr. */
>
> int
> @@ -613,7 +654,8 @@ cp_gimplify_expr (tree *expr_p, gimple_s
> gcc_assert (call_expr_nargs (*expr_p) == 2);
> gcc_assert (!CALL_EXPR_ORDERED_ARGS (*expr_p));
> enum gimplify_status t
> - = gimplify_arg (&CALL_EXPR_ARG (*expr_p, 1), pre_p, loc);
> + = cp_gimplify_arg (&CALL_EXPR_ARG (*expr_p, 1), pre_p, loc,
> + TREE_SIDE_EFFECTS (CALL_EXPR_ARG (*expr_p, 0)));
> if (t == GS_ERROR)
> ret = GS_ERROR;
> }
> @@ -622,10 +664,18 @@ cp_gimplify_expr (tree *expr_p, gimple_s
> /* Leave the last argument for gimplify_call_expr, to avoid problems
> with __builtin_va_arg_pack(). */
> int nargs = call_expr_nargs (*expr_p) - 1;
> + int last_side_effects_arg = -1;
> + for (int i = nargs; i > 0; --i)
> + if (TREE_SIDE_EFFECTS (CALL_EXPR_ARG (*expr_p, i)))
> + {
> + last_side_effects_arg = i;
> + break;
> + }
> for (int i = 0; i < nargs; ++i)
> {
> enum gimplify_status t
> - = gimplify_arg (&CALL_EXPR_ARG (*expr_p, i), pre_p, loc);
> + = cp_gimplify_arg (&CALL_EXPR_ARG (*expr_p, i), pre_p, loc,
> + i < last_side_effects_arg);
> if (t == GS_ERROR)
> ret = GS_ERROR;
> }
> @@ -639,8 +689,17 @@ cp_gimplify_expr (tree *expr_p, gimple_s
> fntype = TREE_TYPE (fntype);
> if (TREE_CODE (fntype) == METHOD_TYPE)
> {
> + int nargs = call_expr_nargs (*expr_p);
> + bool side_effects = false;
> + for (int i = 1; i < nargs; ++i)
> + if (TREE_SIDE_EFFECTS (CALL_EXPR_ARG (*expr_p, i)))
> + {
> + side_effects = true;
> + break;
> + }
> enum gimplify_status t
> - = gimplify_arg (&CALL_EXPR_ARG (*expr_p, 0), pre_p, loc);
> + = cp_gimplify_arg (&CALL_EXPR_ARG (*expr_p, 0), pre_p, loc,
> + side_effects);
> if (t == GS_ERROR)
> ret = GS_ERROR;
> }
> --- gcc/testsuite/g++.dg/cpp1z/eval-order11.C.jj 2021-11-04 14:02:50.439482571 +0100
> +++ gcc/testsuite/g++.dg/cpp1z/eval-order11.C 2021-11-04 14:15:43.850479169 +0100
> @@ -0,0 +1,89 @@
> +// PR c++/70796
> +// { dg-do run { target c++11 } }
> +// { dg-options "-fstrong-eval-order" { target c++14_down } }
> +
> +struct A
> +{
> + int x = 0;
> + A & operator ++ () { ++x; return *this; }
> +};
> +struct B
> +{
> + A first, second;
> + B (A x, A y) : first{x}, second{y} {}
> +};
> +struct C
> +{
> + int first, second;
> + C (int x, int y) : first{x}, second{y} {}
> +};
> +struct D
> +{
> + int d;
> + void foo (int x, D *y)
> + {
> + if (y != this + 1)
> + __builtin_abort ();
> + d = x;
> + }
> +};
> +D d[2] = { { 1 }, { 2 } };
> +
> +void
> +foo ()
> +{
> + int i = 0;
> + C p{++i, ++i};
> + if (p.first != 1 || p.second != 2)
> + __builtin_abort ();
> +}
> +
> +void
> +bar ()
> +{
> + int i = 0;
> + C p{++i, ++i};
> + if (p.first != 1 || p.second != 2)
> + __builtin_abort ();
> + int &j = i;
> + C q{++j, ++j};
> + if (q.first != 3 || q.second != 4)
> + __builtin_abort ();
> +}
> +
> +void
> +baz ()
> +{
> + int i = 0;
> + C p{(int &) ++i, (int &) ++i};
> + if (p.first != 1 || p.second != 2)
> + __builtin_abort ();
> +}
> +
> +void
> +qux ()
> +{
> + A i;
> + B p{++i, ++i};
> + if (p.first.x != 1 || p.second.x != 2)
> + __builtin_abort ();
> +}
> +
> +void
> +corge ()
> +{
> + D *p = &d[0];
> + p->foo (3, ++p);
> + if (d[0].d != 3 || d[1].d != 2)
> + __builtin_abort ();
> +}
> +
> +int
> +main ()
> +{
> + bar ();
> + baz ();
> + foo ();
> + qux ();
> + corge ();
> +}
>
>
> Jakub
>
@@ -398,6 +398,47 @@ gimplify_to_rvalue (tree *expr_p, gimple
return t;
}
+/* Like gimplify_arg, but if ORDERED is set (which should be set if
+ any of the arguments this argument is sequenced before has
+ TREE_SIDE_EFFECTS set, make sure expressions with is_gimple_reg_type type
+ are gimplified into SSA_NAME or a fresh temporary and for
+ non-is_gimple_reg_type we don't optimize away TARGET_EXPRs. */
+
+static enum gimplify_status
+cp_gimplify_arg (tree *arg_p, gimple_seq *pre_p, location_t call_location,
+ bool ordered)
+{
+ enum gimplify_status t;
+ if (ordered
+ && !is_gimple_reg_type (TREE_TYPE (*arg_p))
+ && TREE_CODE (*arg_p) == TARGET_EXPR)
+ {
+ /* gimplify_arg would strip away the TARGET_EXPR, but
+ that can mean we don't copy the argument and some following
+ argument with side-effect could modify it. */
+ protected_set_expr_location (*arg_p, call_location);
+ return gimplify_expr (arg_p, pre_p, NULL, is_gimple_lvalue, fb_either);
+ }
+ else
+ {
+ t = gimplify_arg (arg_p, pre_p, call_location);
+ if (t == GS_ERROR)
+ return GS_ERROR;
+ else if (ordered
+ && is_gimple_reg_type (TREE_TYPE (*arg_p))
+ && is_gimple_variable (*arg_p)
+ && TREE_CODE (*arg_p) != SSA_NAME
+ /* No need to force references into register, references
+ can't be modified. */
+ && !TYPE_REF_P (TREE_TYPE (*arg_p))
+ /* And this can't be modified either. */
+ && *arg_p != current_class_ptr)
+ *arg_p = get_initialized_tmp_var (*arg_p, pre_p);
+ return t;
+ }
+
+}
+
/* Do C++-specific gimplification. Args are as for gimplify_expr. */
int
@@ -613,7 +654,8 @@ cp_gimplify_expr (tree *expr_p, gimple_s
gcc_assert (call_expr_nargs (*expr_p) == 2);
gcc_assert (!CALL_EXPR_ORDERED_ARGS (*expr_p));
enum gimplify_status t
- = gimplify_arg (&CALL_EXPR_ARG (*expr_p, 1), pre_p, loc);
+ = cp_gimplify_arg (&CALL_EXPR_ARG (*expr_p, 1), pre_p, loc,
+ TREE_SIDE_EFFECTS (CALL_EXPR_ARG (*expr_p, 0)));
if (t == GS_ERROR)
ret = GS_ERROR;
}
@@ -622,10 +664,18 @@ cp_gimplify_expr (tree *expr_p, gimple_s
/* Leave the last argument for gimplify_call_expr, to avoid problems
with __builtin_va_arg_pack(). */
int nargs = call_expr_nargs (*expr_p) - 1;
+ int last_side_effects_arg = -1;
+ for (int i = nargs; i > 0; --i)
+ if (TREE_SIDE_EFFECTS (CALL_EXPR_ARG (*expr_p, i)))
+ {
+ last_side_effects_arg = i;
+ break;
+ }
for (int i = 0; i < nargs; ++i)
{
enum gimplify_status t
- = gimplify_arg (&CALL_EXPR_ARG (*expr_p, i), pre_p, loc);
+ = cp_gimplify_arg (&CALL_EXPR_ARG (*expr_p, i), pre_p, loc,
+ i < last_side_effects_arg);
if (t == GS_ERROR)
ret = GS_ERROR;
}
@@ -639,8 +689,17 @@ cp_gimplify_expr (tree *expr_p, gimple_s
fntype = TREE_TYPE (fntype);
if (TREE_CODE (fntype) == METHOD_TYPE)
{
+ int nargs = call_expr_nargs (*expr_p);
+ bool side_effects = false;
+ for (int i = 1; i < nargs; ++i)
+ if (TREE_SIDE_EFFECTS (CALL_EXPR_ARG (*expr_p, i)))
+ {
+ side_effects = true;
+ break;
+ }
enum gimplify_status t
- = gimplify_arg (&CALL_EXPR_ARG (*expr_p, 0), pre_p, loc);
+ = cp_gimplify_arg (&CALL_EXPR_ARG (*expr_p, 0), pre_p, loc,
+ side_effects);
if (t == GS_ERROR)
ret = GS_ERROR;
}
@@ -0,0 +1,89 @@
+// PR c++/70796
+// { dg-do run { target c++11 } }
+// { dg-options "-fstrong-eval-order" { target c++14_down } }
+
+struct A
+{
+ int x = 0;
+ A & operator ++ () { ++x; return *this; }
+};
+struct B
+{
+ A first, second;
+ B (A x, A y) : first{x}, second{y} {}
+};
+struct C
+{
+ int first, second;
+ C (int x, int y) : first{x}, second{y} {}
+};
+struct D
+{
+ int d;
+ void foo (int x, D *y)
+ {
+ if (y != this + 1)
+ __builtin_abort ();
+ d = x;
+ }
+};
+D d[2] = { { 1 }, { 2 } };
+
+void
+foo ()
+{
+ int i = 0;
+ C p{++i, ++i};
+ if (p.first != 1 || p.second != 2)
+ __builtin_abort ();
+}
+
+void
+bar ()
+{
+ int i = 0;
+ C p{++i, ++i};
+ if (p.first != 1 || p.second != 2)
+ __builtin_abort ();
+ int &j = i;
+ C q{++j, ++j};
+ if (q.first != 3 || q.second != 4)
+ __builtin_abort ();
+}
+
+void
+baz ()
+{
+ int i = 0;
+ C p{(int &) ++i, (int &) ++i};
+ if (p.first != 1 || p.second != 2)
+ __builtin_abort ();
+}
+
+void
+qux ()
+{
+ A i;
+ B p{++i, ++i};
+ if (p.first.x != 1 || p.second.x != 2)
+ __builtin_abort ();
+}
+
+void
+corge ()
+{
+ D *p = &d[0];
+ p->foo (3, ++p);
+ if (d[0].d != 3 || d[1].d != 2)
+ __builtin_abort ();
+}
+
+int
+main ()
+{
+ bar ();
+ baz ();
+ foo ();
+ qux ();
+ corge ();
+}