diff mbox series

[v9,08/10] Add tests for C/C++ musttail attributes

Message ID 20240708170031.1621184-9-ak@linux.intel.com
State New
Headers show
Series [v9,01/10] Improve must tail in RTL backend | expand

Commit Message

Andi Kleen July 8, 2024, 4:56 p.m. UTC
Some adopted from the existing C musttail plugin tests.

gcc/testsuite/ChangeLog:

	* c-c++-common/musttail1.c: New test.
	* c-c++-common/musttail2.c: New test.
	* c-c++-common/musttail3.c: New test.
	* c-c++-common/musttail4.c: New test.
	* c-c++-common/musttail7.c: New test.
	* c-c++-common/musttail8.c: New test.
	* g++.dg/musttail6.C: New test.
	* g++.dg/musttail9.C: New test.
	* g++.dg/musttail10.C: New test.
---
 gcc/testsuite/c-c++-common/musttail1.c | 14 ++++++
 gcc/testsuite/c-c++-common/musttail2.c | 33 ++++++++++++++
 gcc/testsuite/c-c++-common/musttail3.c | 29 ++++++++++++
 gcc/testsuite/c-c++-common/musttail4.c | 17 +++++++
 gcc/testsuite/c-c++-common/musttail5.c | 28 ++++++++++++
 gcc/testsuite/c-c++-common/musttail7.c | 14 ++++++
 gcc/testsuite/c-c++-common/musttail8.c | 17 +++++++
 gcc/testsuite/g++.dg/musttail10.C      | 34 ++++++++++++++
 gcc/testsuite/g++.dg/musttail6.C       | 61 ++++++++++++++++++++++++++
 gcc/testsuite/g++.dg/musttail9.C       | 10 +++++
 10 files changed, 257 insertions(+)
 create mode 100644 gcc/testsuite/c-c++-common/musttail1.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail2.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail3.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail4.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail5.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail7.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail8.c
 create mode 100644 gcc/testsuite/g++.dg/musttail10.C
 create mode 100644 gcc/testsuite/g++.dg/musttail6.C
 create mode 100644 gcc/testsuite/g++.dg/musttail9.C

Comments

Jason Merrill July 15, 2024, 10:57 p.m. UTC | #1
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
Andi Kleen July 16, 2024, 3:24 a.m. UTC | #2
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>();
}
Jason Merrill July 16, 2024, 2:32 p.m. UTC | #3
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>();
> }
>
Andi Kleen July 16, 2024, 3:15 p.m. UTC | #4
> 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
Jason Merrill July 16, 2024, 3:17 p.m. UTC | #5
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
Andi Kleen July 16, 2024, 4:18 p.m. UTC | #6
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>();
 }
Jason Merrill July 16, 2024, 6:51 p.m. UTC | #7
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>();
>   }
>
Andi Kleen July 16, 2024, 7:52 p.m. UTC | #8
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>();
 }
Andi Kleen July 16, 2024, 9:55 p.m. UTC | #9
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.  */
Jason Merrill July 16, 2024, 10:06 p.m. UTC | #10
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.  */
>
Andi Kleen July 16, 2024, 10:54 p.m. UTC | #11
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>();
 }
Jason Merrill July 17, 2024, 12:19 a.m. UTC | #12
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>();
>   }
>
Andi Kleen July 17, 2024, 3:18 p.m. UTC | #13
> 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 mbox series

Patch

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" } */
+}