Message ID | 20240708170031.1621184-9-ak@linux.intel.com |
---|---|
State | New |
Headers | show |
Series | [v9,01/10] Improve must tail in RTL backend | expand |
On 7/8/24 12:56 PM, Andi Kleen wrote: > diff --git a/gcc/testsuite/g++.dg/musttail10.C b/gcc/testsuite/g++.dg/musttail10.C > new file mode 100644 > index 000000000000..9b7043b8a306 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/musttail10.C > @@ -0,0 +1,34 @@ > +/* { dg-do compile { target { tail_call } } } */ > +/* { dg-options "-std=gnu++11" } */ > +/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */ > + > +int f(); > + > +double h() { [[gnu::musttail]] return f(); } /* { dg-error "cannot tail-call" } */ > + > +template <class T> > +__attribute__((noinline, noclone, noipa)) > +T g1() { [[gnu::musttail]] return f(); } /* { dg-error "target is not able" "" { target powerpc*-*-* } } */ > + > +template <class T> > +__attribute__((noinline, noclone, noipa)) > +T g2() { [[gnu::musttail]] return f(); } /* { dg-error "cannot tail-call" } */ > + > +template <class T> > +__attribute__((noinline, noclone, noipa)) > +T g3() { [[gnu::musttail]] return f(); } /* { dg-error "cannot tail-call" } */ > + > +class C > +{ > + double x; > +public: > + C(double x) : x(x) {} > + ~C() { asm("":::"memory"); } > +}; > + > +int main() > +{ > + g1<int>(); > + g2<double>(); > + g3<C>(); > +} I had asked for this test to check the case where the function called with [[musttail]] returns a non-trivially-copyable class; the test now includes such a class, but all the [[musttail]] calls are still to a function that returns int. Jason
On Mon, Jul 15, 2024 at 06:57:57PM -0400, Jason Merrill wrote: > On 7/8/24 12:56 PM, Andi Kleen wrote: > > diff --git a/gcc/testsuite/g++.dg/musttail10.C b/gcc/testsuite/g++.dg/musttail10.C > > new file mode 100644 > > index 000000000000..9b7043b8a306 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/musttail10.C > > @@ -0,0 +1,34 @@ > > +/* { dg-do compile { target { tail_call } } } */ > > +/* { dg-options "-std=gnu++11" } */ > > +/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */ > > + > > +int f(); > > + > > +double h() { [[gnu::musttail]] return f(); } /* { dg-error "cannot tail-call" } */ > > + > > +template <class T> > > +__attribute__((noinline, noclone, noipa)) > > +T g1() { [[gnu::musttail]] return f(); } /* { dg-error "target is not able" "" { target powerpc*-*-* } } */ > > + > > +template <class T> > > +__attribute__((noinline, noclone, noipa)) > > +T g2() { [[gnu::musttail]] return f(); } /* { dg-error "cannot tail-call" } */ > > + > > +template <class T> > > +__attribute__((noinline, noclone, noipa)) > > +T g3() { [[gnu::musttail]] return f(); } /* { dg-error "cannot tail-call" } */ > > + > > +class C > > +{ > > + double x; > > +public: > > + C(double x) : x(x) {} > > + ~C() { asm("":::"memory"); } > > +}; > > + > > +int main() > > +{ > > + g1<int>(); > > + g2<double>(); > > + g3<C>(); > > +} > > I had asked for this test to check the case where the function called with > [[musttail]] returns a non-trivially-copyable class; the test now includes > such a class, but all the [[musttail]] calls are still to a function that > returns int. Thanks Jason. I fixed the test case, but now the musttail gets lost, no error for g2/g3. That means the flag is still lost somewhere. Does something outside tsubst need changes too? Right now tsubst has only this: @@ -21113,12 +21113,17 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl) bool op = CALL_EXPR_OPERATOR_SYNTAX (t); bool ord = CALL_EXPR_ORDERED_ARGS (t); bool rev = CALL_EXPR_REVERSE_ARGS (t); - if (op || ord || rev) + bool mtc = false; + if (TREE_CODE (t) == CALL_EXPR) + mtc = CALL_EXPR_MUST_TAIL_CALL (t); + if (op || ord || rev || mtc) if (tree call = extract_call_expr (ret)) { CALL_EXPR_OPERATOR_SYNTAX (call) = op; CALL_EXPR_ORDERED_ARGS (call) = ord; CALL_EXPR_REVERSE_ARGS (call) = rev; + if (TREE_CODE (call) == CALL_EXPR) + CALL_EXPR_MUST_TAIL_CALL (call) = mtc; } if (warning_suppressed_p (t, OPT_Wpessimizing_move)) /* This also suppresses -Wredundant-move. */ Fixed test case: template <class T> T f(); double h() { [[gnu::musttail]] return f<int>(); } /* { dg-error "cannot tail-call" } */ template <class T> __attribute__((noinline, noclone, noipa)) T g1() { [[gnu::musttail]] return f<T>(); } /* { dg-error "target is not able" "" { target powerpc*-*-* } } */ template <class T> __attribute__((noinline, noclone, noipa)) T g2() { [[gnu::musttail]] return f<T>(); } /* { dg-error "cannot tail-call" } */ template <class T> __attribute__((noinline, noclone, noipa)) T g3() { [[gnu::musttail]] return f<T>(); } /* { dg-error "cannot tail-call" } */ class C { double x; public: C(double x) : x(x) {} ~C() { asm("":::"memory"); } }; int main() { g1<int>(); g2<double>(); g3<C>(); }
On 7/15/24 11:24 PM, Andi Kleen wrote: > On Mon, Jul 15, 2024 at 06:57:57PM -0400, Jason Merrill wrote: >> On 7/8/24 12:56 PM, Andi Kleen wrote: >>> diff --git a/gcc/testsuite/g++.dg/musttail10.C b/gcc/testsuite/g++.dg/musttail10.C >>> new file mode 100644 >>> index 000000000000..9b7043b8a306 >>> --- /dev/null >>> +++ b/gcc/testsuite/g++.dg/musttail10.C >>> @@ -0,0 +1,34 @@ >>> +/* { dg-do compile { target { tail_call } } } */ >>> +/* { dg-options "-std=gnu++11" } */ >>> +/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */ >>> + >>> +int f(); >>> + >>> +double h() { [[gnu::musttail]] return f(); } /* { dg-error "cannot tail-call" } */ >>> + >>> +template <class T> >>> +__attribute__((noinline, noclone, noipa)) >>> +T g1() { [[gnu::musttail]] return f(); } /* { dg-error "target is not able" "" { target powerpc*-*-* } } */ >>> + >>> +template <class T> >>> +__attribute__((noinline, noclone, noipa)) >>> +T g2() { [[gnu::musttail]] return f(); } /* { dg-error "cannot tail-call" } */ >>> + >>> +template <class T> >>> +__attribute__((noinline, noclone, noipa)) >>> +T g3() { [[gnu::musttail]] return f(); } /* { dg-error "cannot tail-call" } */ >>> + >>> +class C >>> +{ >>> + double x; >>> +public: >>> + C(double x) : x(x) {} >>> + ~C() { asm("":::"memory"); } >>> +}; >>> + >>> +int main() >>> +{ >>> + g1<int>(); >>> + g2<double>(); >>> + g3<C>(); >>> +} >> >> I had asked for this test to check the case where the function called with >> [[musttail]] returns a non-trivially-copyable class; the test now includes >> such a class, but all the [[musttail]] calls are still to a function that >> returns int. > > Thanks Jason. > > I fixed the test case, but now the musttail gets lost, no error for g2/g3. > > That means the flag is still lost somewhere. Does something outside tsubst need changes too? > > Right now tsubst has only this: > > @@ -21113,12 +21113,17 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl) > bool op = CALL_EXPR_OPERATOR_SYNTAX (t); > bool ord = CALL_EXPR_ORDERED_ARGS (t); > bool rev = CALL_EXPR_REVERSE_ARGS (t); > - if (op || ord || rev) > + bool mtc = false; > + if (TREE_CODE (t) == CALL_EXPR) > + mtc = CALL_EXPR_MUST_TAIL_CALL (t); > + if (op || ord || rev || mtc) > if (tree call = extract_call_expr (ret)) > { > CALL_EXPR_OPERATOR_SYNTAX (call) = op; > CALL_EXPR_ORDERED_ARGS (call) = ord; > CALL_EXPR_REVERSE_ARGS (call) = rev; > + if (TREE_CODE (call) == CALL_EXPR) > + CALL_EXPR_MUST_TAIL_CALL (call) = mtc; > } > if (warning_suppressed_p (t, OPT_Wpessimizing_move)) > /* This also suppresses -Wredundant-move. */ > > > Fixed test case: > > > template <class T> T f(); > > double h() { [[gnu::musttail]] return f<int>(); } /* { dg-error "cannot tail-call" } */ > > template <class T> > __attribute__((noinline, noclone, noipa)) > T g1() { [[gnu::musttail]] return f<T>(); } /* { dg-error "target is not able" "" { target powerpc*-*-* } } */ > > template <class T> > __attribute__((noinline, noclone, noipa)) > T g2() { [[gnu::musttail]] return f<T>(); } /* { dg-error "cannot tail-call" } */ > > template <class T> > __attribute__((noinline, noclone, noipa)) > T g3() { [[gnu::musttail]] return f<T>(); } /* { dg-error "cannot tail-call" } */ In the adjusted test it looks like the types of f and g match, so I wouldn't expect an error. > class C > { > double x; > public: > C(double x) : x(x) {} > ~C() { asm("":::"memory"); } > }; > > int main() > { > g1<int>(); > g2<double>(); > g3<C>(); > } >
> In the adjusted test it looks like the types of f and g match, so I wouldn't > expect an error. Good point! Missing the forest for the trees. Anyways are the C++ patches ok with this change? Thanks, -Andi
On 7/16/24 11:15 AM, Andi Kleen wrote: >> In the adjusted test it looks like the types of f and g match, so I wouldn't >> expect an error. > > Good point! Missing the forest for the trees. > > Anyways are the C++ patches ok with this change? I'm still looking for a test which does error because the types are different. Jason
On Tue, Jul 16, 2024 at 11:17:14AM -0400, Jason Merrill wrote: > On 7/16/24 11:15 AM, Andi Kleen wrote: > > > In the adjusted test it looks like the types of f and g match, so I wouldn't > > > expect an error. > > > > Good point! Missing the forest for the trees. > > > > Anyways are the C++ patches ok with this change? > > I'm still looking for a test which does error because the types are > different. Like this? -Andi diff --git a/gcc/testsuite/g++.dg/musttail10.C b/gcc/testsuite/g++.dg/musttail10.C index 6a8507784a14..e454a6238a06 100644 --- a/gcc/testsuite/g++.dg/musttail10.C +++ b/gcc/testsuite/g++.dg/musttail10.C @@ -4,7 +4,7 @@ template <class T> T f(); -double h() { [[gnu::musttail]] return f<int>(); } /* { dg-error "cannot tail-call" } */ +double g() { [[gnu::musttail]] return f<int>(); } /* { dg-error "cannot tail-call" } */ template <class T> __attribute__((noinline, noclone, noipa)) @@ -18,6 +18,10 @@ template <class T> __attribute__((noinline, noclone, noipa)) T g3() { [[gnu::musttail]] return f<T>(); } +template <class T> +__attribute__((noinline, noclone, noipa)) +T g4() { [[gnu::musttail]] return f<double>(); } /* { dg-error "cannot tail-call" } */ + class C { double x; @@ -31,4 +35,5 @@ int main() g1<int>(); g2<double>(); g3<C>(); + g4<int>(); }
On 7/16/24 12:18 PM, Andi Kleen wrote: > On Tue, Jul 16, 2024 at 11:17:14AM -0400, Jason Merrill wrote: >> On 7/16/24 11:15 AM, Andi Kleen wrote: >>>> In the adjusted test it looks like the types of f and g match, so I wouldn't >>>> expect an error. >>> >>> Good point! Missing the forest for the trees. >>> >>> Anyways are the C++ patches ok with this change? >> >> I'm still looking for a test which does error because the types are >> different. > > Like this? Where the called function returns C and the callee function does not. > -Andi > > > diff --git a/gcc/testsuite/g++.dg/musttail10.C b/gcc/testsuite/g++.dg/musttail10.C > index 6a8507784a14..e454a6238a06 100644 > --- a/gcc/testsuite/g++.dg/musttail10.C > +++ b/gcc/testsuite/g++.dg/musttail10.C > @@ -4,7 +4,7 @@ > > template <class T> T f(); > > -double h() { [[gnu::musttail]] return f<int>(); } /* { dg-error "cannot tail-call" } */ > +double g() { [[gnu::musttail]] return f<int>(); } /* { dg-error "cannot tail-call" } */ > > template <class T> > __attribute__((noinline, noclone, noipa)) > @@ -18,6 +18,10 @@ template <class T> > __attribute__((noinline, noclone, noipa)) > T g3() { [[gnu::musttail]] return f<T>(); } > > +template <class T> > +__attribute__((noinline, noclone, noipa)) > +T g4() { [[gnu::musttail]] return f<double>(); } /* { dg-error "cannot tail-call" } */ > + > class C > { > double x; > @@ -31,4 +35,5 @@ int main() > g1<int>(); > g2<double>(); > g3<C>(); > + g4<int>(); > } >
On Tue, Jul 16, 2024 at 02:51:13PM -0400, Jason Merrill wrote: > On 7/16/24 12:18 PM, Andi Kleen wrote: > > On Tue, Jul 16, 2024 at 11:17:14AM -0400, Jason Merrill wrote: > > > On 7/16/24 11:15 AM, Andi Kleen wrote: > > > > > In the adjusted test it looks like the types of f and g match, so I wouldn't > > > > > expect an error. > > > > > > > > Good point! Missing the forest for the trees. > > > > > > > > Anyways are the C++ patches ok with this change? > > > > > > I'm still looking for a test which does error because the types are > > > different. > > > > Like this? > > Where the called function returns C and the callee function does not. In this case the attribute seems to get lost and it succeeds. diff --git a/gcc/testsuite/g++.dg/musttail10.C b/gcc/testsuite/g++.dg/musttail10.C index e454a6238a06..39f0ec38253d 100644 --- a/gcc/testsuite/g++.dg/musttail10.C +++ b/gcc/testsuite/g++.dg/musttail10.C @@ -28,12 +28,18 @@ class C public: C(double x) : x(x) {} ~C() { asm("":::"memory"); } + operator int() { return x; } }; +template <class T> +__attribute__((noinline, noclone, noipa)) +T g5() { [[gnu::musttail]] return f<C>(); } /* { dg-error "cannot tail-call" } */ + int main() { g1<int>(); g2<double>(); g3<C>(); g4<int>(); + g5<int>(); }
On Tue, Jul 16, 2024 at 12:52:31PM -0700, Andi Kleen wrote: > On Tue, Jul 16, 2024 at 02:51:13PM -0400, Jason Merrill wrote: > > On 7/16/24 12:18 PM, Andi Kleen wrote: > > > On Tue, Jul 16, 2024 at 11:17:14AM -0400, Jason Merrill wrote: > > > > On 7/16/24 11:15 AM, Andi Kleen wrote: > > > > > > In the adjusted test it looks like the types of f and g match, so I wouldn't > > > > > > expect an error. > > > > > > > > > > Good point! Missing the forest for the trees. > > > > > > > > > > Anyways are the C++ patches ok with this change? > > > > > > > > I'm still looking for a test which does error because the types are > > > > different. > > > > > > Like this? > > > > Where the called function returns C and the callee function does not. > > In this case the attribute seems to get lost and it succeeds. This somewhat hackish patch fixes it here, to handle the case of a TARGET_EXPR where the CALL_EXPR is in the cleanup. extract_call bails on that. diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index 3b914089a6e2..8753aa51da52 100644 --- a/gcc/cp/pt.cc +++ b/gcc/cp/pt.cc @@ -21124,6 +21124,10 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl) CALL_EXPR_REVERSE_ARGS (call) = rev; if (TREE_CODE (call) == CALL_EXPR) CALL_EXPR_MUST_TAIL_CALL (call) = mtc; + else if (mtc + && TREE_CODE (ret) == TARGET_EXPR + && TREE_CODE (TARGET_EXPR_CLEANUP (ret)) == CALL_EXPR) + CALL_EXPR_MUST_TAIL_CALL (TARGET_EXPR_CLEANUP (ret)) = mtc; } if (warning_suppressed_p (t, OPT_Wpessimizing_move)) /* This also suppresses -Wredundant-move. */
On 7/16/24 5:55 PM, Andi Kleen wrote: > On Tue, Jul 16, 2024 at 12:52:31PM -0700, Andi Kleen wrote: >> On Tue, Jul 16, 2024 at 02:51:13PM -0400, Jason Merrill wrote: >>> On 7/16/24 12:18 PM, Andi Kleen wrote: >>>> On Tue, Jul 16, 2024 at 11:17:14AM -0400, Jason Merrill wrote: >>>>> On 7/16/24 11:15 AM, Andi Kleen wrote: >>>>>>> In the adjusted test it looks like the types of f and g match, so I wouldn't >>>>>>> expect an error. >>>>>> >>>>>> Good point! Missing the forest for the trees. >>>>>> >>>>>> Anyways are the C++ patches ok with this change? >>>>> >>>>> I'm still looking for a test which does error because the types are >>>>> different. >>>> >>>> Like this? >>> >>> Where the called function returns C and the callee function does not. >> >> In this case the attribute seems to get lost and it succeeds. > > This somewhat hackish patch fixes it here, to handle the case > of a TARGET_EXPR where the CALL_EXPR is in the cleanup. extract_call > bails on that. The CALL_EXPR in the cleanup is calling the destructor, that's not what we're trying to tail-call. I think the problem here is that the call to f<C> is represented with an AGGR_INIT_EXPR instead of CALL_EXPR, so you need to handle the flag on that tree_code as well. > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index 3b914089a6e2..8753aa51da52 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -21124,6 +21124,10 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl) > CALL_EXPR_REVERSE_ARGS (call) = rev; > if (TREE_CODE (call) == CALL_EXPR) > CALL_EXPR_MUST_TAIL_CALL (call) = mtc; > + else if (mtc > + && TREE_CODE (ret) == TARGET_EXPR > + && TREE_CODE (TARGET_EXPR_CLEANUP (ret)) == CALL_EXPR) > + CALL_EXPR_MUST_TAIL_CALL (TARGET_EXPR_CLEANUP (ret)) = mtc; > } > if (warning_suppressed_p (t, OPT_Wpessimizing_move)) > /* This also suppresses -Wredundant-move. */ >
On Tue, Jul 16, 2024 at 06:06:42PM -0400, Jason Merrill wrote: > On 7/16/24 5:55 PM, Andi Kleen wrote: > > On Tue, Jul 16, 2024 at 12:52:31PM -0700, Andi Kleen wrote: > > > On Tue, Jul 16, 2024 at 02:51:13PM -0400, Jason Merrill wrote: > > > > On 7/16/24 12:18 PM, Andi Kleen wrote: > > > > > On Tue, Jul 16, 2024 at 11:17:14AM -0400, Jason Merrill wrote: > > > > > > On 7/16/24 11:15 AM, Andi Kleen wrote: > > > > > > > > In the adjusted test it looks like the types of f and g match, so I wouldn't > > > > > > > > expect an error. > > > > > > > > > > > > > > Good point! Missing the forest for the trees. > > > > > > > > > > > > > > Anyways are the C++ patches ok with this change? > > > > > > > > > > > > I'm still looking for a test which does error because the types are > > > > > > different. > > > > > > > > > > Like this? > > > > > > > > Where the called function returns C and the callee function does not. > > > > > > In this case the attribute seems to get lost and it succeeds. > > > > This somewhat hackish patch fixes it here, to handle the case > > of a TARGET_EXPR where the CALL_EXPR is in the cleanup. extract_call > > bails on that. > > The CALL_EXPR in the cleanup is calling the destructor, that's not what > we're trying to tail-call. > > I think the problem here is that the call to f<C> is represented with an > AGGR_INIT_EXPR instead of CALL_EXPR, so you need to handle the flag on that > tree_code as well. Okay this seems to work (I had to adjust the test case because it now correctly errors out on passing the class at -O0) diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 4bb3e9c4989b..5ec8102c1849 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -4245,6 +4245,10 @@ templated_operator_saved_lookups (tree t) #define AGGR_INIT_FROM_THUNK_P(NODE) \ (AGGR_INIT_EXPR_CHECK (NODE)->base.protected_flag) +/* Nonzero means that the call was marked musttail. */ +#define AGGR_INIT_EXPR_MUST_TAIL(NODE) \ + (AGGR_INIT_EXPR_CHECK (NODE)->base.static_flag) + /* AGGR_INIT_EXPR accessors. These are equivalent to the CALL_EXPR accessors, except for AGGR_INIT_EXPR_SLOT (which takes the place of CALL_EXPR_STATIC_CHAIN). */ diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index 3b914089a6e2..d668c5af6a23 100644 --- a/gcc/cp/pt.cc +++ b/gcc/cp/pt.cc @@ -21124,6 +21124,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl) CALL_EXPR_REVERSE_ARGS (call) = rev; if (TREE_CODE (call) == CALL_EXPR) CALL_EXPR_MUST_TAIL_CALL (call) = mtc; + else if (TREE_CODE (call) == AGGR_INIT_EXPR) + AGGR_INIT_EXPR_MUST_TAIL (call) = mtc; } if (warning_suppressed_p (t, OPT_Wpessimizing_move)) /* This also suppresses -Wredundant-move. */ diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index cd3df13772db..fb45974cd90f 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -4979,6 +4979,7 @@ simplify_aggr_init_expr (tree *tp) = CALL_EXPR_OPERATOR_SYNTAX (aggr_init_expr); CALL_EXPR_ORDERED_ARGS (call_expr) = CALL_EXPR_ORDERED_ARGS (aggr_init_expr); CALL_EXPR_REVERSE_ARGS (call_expr) = CALL_EXPR_REVERSE_ARGS (aggr_init_expr); + CALL_EXPR_MUST_TAIL_CALL (call_expr) = AGGR_INIT_EXPR_MUST_TAIL (aggr_init_expr); if (style == ctor) { diff --git a/gcc/testsuite/g++.dg/musttail10.C b/gcc/testsuite/g++.dg/musttail10.C index e454a6238a06..93ec32db160a 100644 --- a/gcc/testsuite/g++.dg/musttail10.C +++ b/gcc/testsuite/g++.dg/musttail10.C @@ -14,9 +14,11 @@ template <class T> __attribute__((noinline, noclone, noipa)) T g2() { [[gnu::musttail]] return f<T>(); } +#if __OPTIMIZE__ >= 1 template <class T> __attribute__((noinline, noclone, noipa)) T g3() { [[gnu::musttail]] return f<T>(); } +#endif template <class T> __attribute__((noinline, noclone, noipa)) @@ -28,12 +30,20 @@ class C public: C(double x) : x(x) {} ~C() { asm("":::"memory"); } + operator int() { return x; } }; +template <class T> +__attribute__((noinline, noclone, noipa)) +T g5() { [[gnu::musttail]] return f<C>(); } /* { dg-error "cannot tail-call" } */ + int main() { g1<int>(); g2<double>(); +#if __OPTIMIZE__ >= 1 g3<C>(); +#endif g4<int>(); + g5<int>(); }
On 7/16/24 6:54 PM, Andi Kleen wrote: > On Tue, Jul 16, 2024 at 06:06:42PM -0400, Jason Merrill wrote: >> On 7/16/24 5:55 PM, Andi Kleen wrote: >>> On Tue, Jul 16, 2024 at 12:52:31PM -0700, Andi Kleen wrote: >>>> On Tue, Jul 16, 2024 at 02:51:13PM -0400, Jason Merrill wrote: >>>>> On 7/16/24 12:18 PM, Andi Kleen wrote: >>>>>> On Tue, Jul 16, 2024 at 11:17:14AM -0400, Jason Merrill wrote: >>>>>>> On 7/16/24 11:15 AM, Andi Kleen wrote: >>>>>>>>> In the adjusted test it looks like the types of f and g match, so I wouldn't >>>>>>>>> expect an error. >>>>>>>> >>>>>>>> Good point! Missing the forest for the trees. >>>>>>>> >>>>>>>> Anyways are the C++ patches ok with this change? >>>>>>> >>>>>>> I'm still looking for a test which does error because the types are >>>>>>> different. >>>>>> >>>>>> Like this? >>>>> >>>>> Where the called function returns C and the callee function does not. >>>> >>>> In this case the attribute seems to get lost and it succeeds. >>> >>> This somewhat hackish patch fixes it here, to handle the case >>> of a TARGET_EXPR where the CALL_EXPR is in the cleanup. extract_call >>> bails on that. >> >> The CALL_EXPR in the cleanup is calling the destructor, that's not what >> we're trying to tail-call. >> >> I think the problem here is that the call to f<C> is represented with an >> AGGR_INIT_EXPR instead of CALL_EXPR, so you need to handle the flag on that >> tree_code as well. > > Okay this seems to work > (I had to adjust the test case because it now correctly errors out > on passing the class at -O0) Great. Does it also work in a non-template function? > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index 4bb3e9c4989b..5ec8102c1849 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -4245,6 +4245,10 @@ templated_operator_saved_lookups (tree t) > #define AGGR_INIT_FROM_THUNK_P(NODE) \ > (AGGR_INIT_EXPR_CHECK (NODE)->base.protected_flag) > > +/* Nonzero means that the call was marked musttail. */ > +#define AGGR_INIT_EXPR_MUST_TAIL(NODE) \ > + (AGGR_INIT_EXPR_CHECK (NODE)->base.static_flag) > + > /* AGGR_INIT_EXPR accessors. These are equivalent to the CALL_EXPR > accessors, except for AGGR_INIT_EXPR_SLOT (which takes the place of > CALL_EXPR_STATIC_CHAIN). */ > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index 3b914089a6e2..d668c5af6a23 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -21124,6 +21124,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl) > CALL_EXPR_REVERSE_ARGS (call) = rev; > if (TREE_CODE (call) == CALL_EXPR) > CALL_EXPR_MUST_TAIL_CALL (call) = mtc; > + else if (TREE_CODE (call) == AGGR_INIT_EXPR) > + AGGR_INIT_EXPR_MUST_TAIL (call) = mtc; > } > if (warning_suppressed_p (t, OPT_Wpessimizing_move)) > /* This also suppresses -Wredundant-move. */ > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index cd3df13772db..fb45974cd90f 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -4979,6 +4979,7 @@ simplify_aggr_init_expr (tree *tp) > = CALL_EXPR_OPERATOR_SYNTAX (aggr_init_expr); > CALL_EXPR_ORDERED_ARGS (call_expr) = CALL_EXPR_ORDERED_ARGS (aggr_init_expr); > CALL_EXPR_REVERSE_ARGS (call_expr) = CALL_EXPR_REVERSE_ARGS (aggr_init_expr); > + CALL_EXPR_MUST_TAIL_CALL (call_expr) = AGGR_INIT_EXPR_MUST_TAIL (aggr_init_expr); > > if (style == ctor) > { > diff --git a/gcc/testsuite/g++.dg/musttail10.C b/gcc/testsuite/g++.dg/musttail10.C > index e454a6238a06..93ec32db160a 100644 > --- a/gcc/testsuite/g++.dg/musttail10.C > +++ b/gcc/testsuite/g++.dg/musttail10.C > @@ -14,9 +14,11 @@ template <class T> > __attribute__((noinline, noclone, noipa)) > T g2() { [[gnu::musttail]] return f<T>(); } > > +#if __OPTIMIZE__ >= 1 > template <class T> > __attribute__((noinline, noclone, noipa)) > T g3() { [[gnu::musttail]] return f<T>(); } > +#endif > > template <class T> > __attribute__((noinline, noclone, noipa)) > @@ -28,12 +30,20 @@ class C > public: > C(double x) : x(x) {} > ~C() { asm("":::"memory"); } > + operator int() { return x; } > }; > > +template <class T> > +__attribute__((noinline, noclone, noipa)) > +T g5() { [[gnu::musttail]] return f<C>(); } /* { dg-error "cannot tail-call" } */ > + > int main() > { > g1<int>(); > g2<double>(); > +#if __OPTIMIZE__ >= 1 > g3<C>(); > +#endif > g4<int>(); > + g5<int>(); > } >
> Great. Does it also work in a non-template function?
Sadly it did not because there needs to be more AGGR_VIEW_EXPR handling,
as you predicted at some point. I fixed it now. Will send updated patches.
-Andi
diff --git a/gcc/testsuite/c-c++-common/musttail1.c b/gcc/testsuite/c-c++-common/musttail1.c new file mode 100644 index 000000000000..74efcc2a0bc6 --- /dev/null +++ b/gcc/testsuite/c-c++-common/musttail1.c @@ -0,0 +1,14 @@ +/* { dg-do compile { target { tail_call && { c || c++11 } } } } */ +/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */ + +int __attribute__((noinline,noclone,noipa)) +callee (int i) +{ + return i * i; +} + +int __attribute__((noinline,noclone,noipa)) +caller (int i) +{ + [[gnu::musttail]] return callee (i + 1); +} diff --git a/gcc/testsuite/c-c++-common/musttail2.c b/gcc/testsuite/c-c++-common/musttail2.c new file mode 100644 index 000000000000..86f2c3d77404 --- /dev/null +++ b/gcc/testsuite/c-c++-common/musttail2.c @@ -0,0 +1,33 @@ +/* { dg-do compile { target { tail_call && { c || c++11 } } } } */ + +struct box { char field[256]; int i; }; + +int __attribute__((noinline,noclone,noipa)) +test_2_callee (int i, struct box b) +{ + if (b.field[0]) + return 5; + return i * i; +} + +int __attribute__((noinline,noclone,noipa)) +test_2_caller (int i) +{ + struct box b; + [[gnu::musttail]] return test_2_callee (i + 1, b); /* { dg-error "cannot tail-call: " } */ +} + +extern void setjmp (void); +void +test_3 (void) +{ + [[gnu::musttail]] return setjmp (); /* { dg-error "cannot tail-call: " } */ +} + +extern float f7(void); + +int +test_6 (void) +{ + [[gnu::musttail]] return f7(); /* { dg-error "cannot tail-call: " } */ +} diff --git a/gcc/testsuite/c-c++-common/musttail3.c b/gcc/testsuite/c-c++-common/musttail3.c new file mode 100644 index 000000000000..ea9589c59ef2 --- /dev/null +++ b/gcc/testsuite/c-c++-common/musttail3.c @@ -0,0 +1,29 @@ +/* { dg-do compile { target { tail_call && { c || c++11 } } } } */ + +extern int foo2 (int x, ...); + +struct str +{ + int a, b; +}; + +struct str +cstruct (int x) +{ + if (x < 10) + [[clang::musttail]] return cstruct (x + 1); + return ((struct str){ x, 0 }); +} + +int +foo (int x) +{ + if (x < 10) + [[clang::musttail]] return foo2 (x, 29); + if (x < 100) + { + int k = foo (x + 1); + [[clang::musttail]] return k; /* { dg-error "cannot tail-call: " } */ + } + return x; +} diff --git a/gcc/testsuite/c-c++-common/musttail4.c b/gcc/testsuite/c-c++-common/musttail4.c new file mode 100644 index 000000000000..23f4b5e1cd68 --- /dev/null +++ b/gcc/testsuite/c-c++-common/musttail4.c @@ -0,0 +1,17 @@ +/* { dg-do compile { target { tail_call && { c || c++11 } } } } */ + +struct box { char field[64]; int i; }; + +struct box __attribute__((noinline,noclone,noipa)) +returns_struct (int i) +{ + struct box b; + b.i = i * i; + return b; +} + +int __attribute__((noinline,noclone)) +test_1 (int i) +{ + [[gnu::musttail]] return returns_struct (i * 5).i; /* { dg-error "cannot tail-call: " } */ +} diff --git a/gcc/testsuite/c-c++-common/musttail5.c b/gcc/testsuite/c-c++-common/musttail5.c new file mode 100644 index 000000000000..234da0d3f2a9 --- /dev/null +++ b/gcc/testsuite/c-c++-common/musttail5.c @@ -0,0 +1,28 @@ +/* { dg-do compile } */ +/* { dg-options "-std=c23" { target c } } */ +/* { dg-options "-std=gnu++11" { target c++ } } */ + +[[musttail]] int j; /* { dg-warning "attribute" } */ +__attribute__((musttail)) int k; /* { dg-warning "attribute" } */ + +void foo(void) +{ + [[gnu::musttail]] j++; /* { dg-warning "attribute" } */ + [[gnu::musttail]] if (k > 0) /* { dg-warning "attribute" } */ + [[gnu::musttail]] k++; /* { dg-warning "attribute" } */ +} + +int foo2(int p) +{ + [[gnu::musttail(1)]] return foo2(p + 1); /* { dg-error "\(before numeric constant|attribute\)" } */ +} + +int i; + +int foo3(void) +{ + [[musttail]] i++; /* { dg-warning "attribute" } */ + [[musttail]] if (i > 10) /* { dg-warning "attribute" } */ + [[musttail]] return foo2(i); /* { dg-warning "attribute" } */ + return 0; +} diff --git a/gcc/testsuite/c-c++-common/musttail7.c b/gcc/testsuite/c-c++-common/musttail7.c new file mode 100644 index 000000000000..c753a3fe9b2a --- /dev/null +++ b/gcc/testsuite/c-c++-common/musttail7.c @@ -0,0 +1,14 @@ +/* { dg-do compile { target { tail_call && { c || c++11 } } } } */ +/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */ + +void __attribute__((noipa)) f() {} + +void f2() +{ + [[gnu::musttail]] return f2(); +} + +void f3() +{ + [[gnu::musttail]] return f(); +} diff --git a/gcc/testsuite/c-c++-common/musttail8.c b/gcc/testsuite/c-c++-common/musttail8.c new file mode 100644 index 000000000000..9fa10e0b54c4 --- /dev/null +++ b/gcc/testsuite/c-c++-common/musttail8.c @@ -0,0 +1,17 @@ +/* { dg-do compile { target { tail_call && { c || c++11 } } } } */ + +float f1(void); + +int f2(void) +{ + [[gnu::musttail]] return f1 (); /* { dg-error "changed after call" } */ +} + + +int f3(int *); + +int f4(void) +{ + int x; + [[gnu::musttail]] return f3(&x); /* { dg-error "\(refers to locals|other reasons\)" } */ +} diff --git a/gcc/testsuite/g++.dg/musttail10.C b/gcc/testsuite/g++.dg/musttail10.C new file mode 100644 index 000000000000..9b7043b8a306 --- /dev/null +++ b/gcc/testsuite/g++.dg/musttail10.C @@ -0,0 +1,34 @@ +/* { dg-do compile { target { tail_call } } } */ +/* { dg-options "-std=gnu++11" } */ +/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */ + +int f(); + +double h() { [[gnu::musttail]] return f(); } /* { dg-error "cannot tail-call" } */ + +template <class T> +__attribute__((noinline, noclone, noipa)) +T g1() { [[gnu::musttail]] return f(); } /* { dg-error "target is not able" "" { target powerpc*-*-* } } */ + +template <class T> +__attribute__((noinline, noclone, noipa)) +T g2() { [[gnu::musttail]] return f(); } /* { dg-error "cannot tail-call" } */ + +template <class T> +__attribute__((noinline, noclone, noipa)) +T g3() { [[gnu::musttail]] return f(); } /* { dg-error "cannot tail-call" } */ + +class C +{ + double x; +public: + C(double x) : x(x) {} + ~C() { asm("":::"memory"); } +}; + +int main() +{ + g1<int>(); + g2<double>(); + g3<C>(); +} diff --git a/gcc/testsuite/g++.dg/musttail6.C b/gcc/testsuite/g++.dg/musttail6.C new file mode 100644 index 000000000000..7b5d623af553 --- /dev/null +++ b/gcc/testsuite/g++.dg/musttail6.C @@ -0,0 +1,61 @@ +/* { dg-do compile { target { tail_call } } } */ +/* { dg-skip-if "powerpc does not support sibcall to templates" { powerpc*-*-* } } */ +/* May affect more architectures: */ +/* { dg-skip-if "PR115606, PR115607" { "arm*-*-*" "aarch*-*-*" } } */ +/* { dg-options "-std=gnu++11" } */ +/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */ + +class Foo { +public: + int a, b; + Foo(int a, int b) : a(a), b(b) {} +}; + +Foo __attribute__((noinline,noclone,noipa)) +callee (int i) +{ + return Foo(i, i+1); +} + +Foo __attribute__((noinline,noclone,noipa)) +caller (int i) +{ + [[gnu::musttail]] return callee (i + 1); +} + +template<typename T> +T __attribute__((noinline,noclone,noipa)) foo (T i) +{ + return i + 1; +} + +int +caller2 (int k) +{ + [[gnu::musttail]] return foo<int>(1); +} + +template<typename T> +T caller3 (T v) +{ + [[gnu::musttail]] return foo<T>(v); +} + +int call3(int i) +{ + [[gnu::musttail]] return caller3<int>(i + 1); +} + +struct Bar { + int a; + Bar(int a) : a(a) {} + Bar operator+(Bar o) { return Bar(a + o.a); } +}; + +#if __OPTIMIZE__ >= 1 +Bar +caller4 (Bar k) +{ + [[gnu::musttail]] return caller3<Bar>(Bar(99)); +} +#endif diff --git a/gcc/testsuite/g++.dg/musttail9.C b/gcc/testsuite/g++.dg/musttail9.C new file mode 100644 index 000000000000..fb0262e751be --- /dev/null +++ b/gcc/testsuite/g++.dg/musttail9.C @@ -0,0 +1,10 @@ +/* { dg-do compile { target { tail_call } } } */ +/* { dg-options "-std=gnu++11" } */ +/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */ + +extern void foo(); + +void f() noexcept +{ + [[gnu::musttail]] return foo(); /* { dg-error "call may throw exception that does not propagate" } */ +}