[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Paweł Bylica via Phabricator via cfe-commits]

chfast added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:5320
+  if (!(Cleanup && Cleanup->getCleanup()->isRedundantBeforeReturn()))
+CGM.ErrorUnsupported(MustTailCall, "tail call skipping over cleanups");
+}

I reported a related issue. I wander if this is easy to fix. 
https://github.com/llvm/llvm-project/issues/53087.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

It's not generically true that "anything can be tail-called if it's 
`noreturn`".  For one, `noreturn` doesn't imply that the function doesn't exit 
by e.g. throwing or calling `longjmp`.  For another, the most important user 
expectation of tail calls is that a long series of tail calls will exhibit zero 
overall stack growth; in a caller-pop calling convention, calling a function 
with more parameters may require growing the argument area in a way that cannot 
be reversed, so e.g. a long sequence of tail calls alternating between 
1-argument and 2-argument functions will eventually exhaust the stack, which 
violates that user expectation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

Please also see https://bugs.llvm.org/show_bug.cgi?id=51416


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[David Chisnall via Phabricator via cfe-commits]

theraven added a comment.

Here's a minimal test:

  void tail(int, float);
  
  __attribute__((always_inline))
  void caller(float x)
  {
[[clang::musttail]]
return tail(42, x);
  }
  
  void outer(int x, float y)
  {
  return caller(y);
  }

This raises this error:

  tail.cc:7:3: error: cannot perform a tail call to function 'tail' because its 
signature is incompatible with the calling function
return tail(42, x);
^
  tail.cc:1:1: note: target function has different number of parameters 
(expected 1 but has 2)
  void tail(int, float);
  ^
  tail.cc:6:5: note: tail call required by 'musttail' attribute here
[[clang::musttail]]
  ^

There's also an interesting counterexample:

  void tail(int, float);
  
  __attribute__((always_inline))
  void caller(int a, float x)
  {
[[clang::musttail]]
return tail(a, x);
  }
  
  void outer(float y)
  {
  return caller(42, y);
  }

This *is* accepted by clang, but then generates this IR at -O0:

  define dso_local void @_Z5outerf(float %0) #2 {
%2 = alloca i32, align 4
%3 = alloca float, align 4
%4 = alloca float, align 4
store float %0, float* %4, align 4
%5 = load float, float* %4, align 4
store i32 42, i32* %2, align 4
store float %5, float* %3, align 4
%6 = load i32, i32* %2, align 4
%7 = load float, float* %3, align 4
call void @_Z4tailif(i32 %6, float %7)
ret void
  }

And this IR at -O1:

  ; Function Attrs: uwtable mustprogress
  define dso_local void @_Z5outerf(float %0) local_unnamed_addr #2 {
call void @_Z4tailif(i32 42, float %0)
ret void
  }

Note that in both cases, the alway-inline attribute is respected (even at -O0, 
the always-inline inliner runs) but the musttail annotation is lost.  The 
inlining has inserted the call into a function with a different set of 
parameters and so it cannot have a `musttail` IR annotation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added a comment.

@theraven: Can you post a minimal repro of your case? I don't follow your 
distinction between "caller" and "enclosing function."

Regarding `noreturn` and `always_inline`: maybe the rules for `musttail` could 
be relaxed in cases like the one you mention, but it would require changing the 
backend (LLVM). Here I changed the front-end only and used LLVM's existing 
`musttail` support, which meant accepting its existing limitations 
.

I would love to see an exception for `always_inline`: my use case would benefit 
greatly from this. In my own project I had to change a bunch of `always_inline` 
functions to macros to work around this rule. Unfortunately this is complicated 
by the fact that `always_inline` does not actually guarantee that inlining 
occurs 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[David Chisnall via Phabricator via cfe-commits]

theraven added a comment.

The error message here is very confusing:

  /home/theraven/snmalloc2/src/mem/../ds/../aal/../ds/defines.h:122:27: error: 
cannot perform a tail call to function 'error' because its signature is 
incompatible with the calling function
[[clang::musttail]] return snmalloc::error(str);
^
  /home/theraven/snmalloc2/src/mem/../ds/../aal/../ds/defines.h:63:16: note: 
target function has different number of parameters (expected 2 but has 1)
[[noreturn]] SNMALLOC_COLD void error(const char* const str);
 ^
  /home/theraven/snmalloc2/src/mem/../ds/../aal/../ds/defines.h:21:25: note: 
expanded from macro 'SNMALLOC_COLD'
  #  define SNMALLOC_COLD __attribute__((cold))
  ^
  /home/theraven/snmalloc2/src/mem/../ds/../aal/../ds/defines.h:122:9: note: 
tail call required by 'musttail' attribute here
[[clang::musttail]] return snmalloc::error(str);
  ^

The caller and callee both have one argument, the error is because the 
enclosing function has two parameters.  The error appears wrong anyway for two 
reasons in this particular context:

- The callee is `[[noreturn]]`, so the stack layout doesn't make any 
difference, anything can be tail called if it's no-return.
- The enclosing function is `always_inline`, so checking its argument-frame 
layout does not give useful information because it's the caller's 
argument-frame layout that matters.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

That is a great feature, thank you. Compiling state machines and scheme 
programs to C is now much prettier.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D99517#2693418 , @thakis wrote:

> Looks like this breaks tests on mac/arm: 
> http://45.33.8.238/macm1/7552/step_7.txt

Should be fixed by rGf7c9de0de5804498085af973dc6bfc934a18f000 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Looks like this breaks tests on mac/arm: 
http://45.33.8.238/macm1/7552/step_7.txt

Please take a look and revert for now if it takes a while to fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG834467590842: Implemented [[clang::musttail]] attribute for 
guaranteed tail calls. (authored by haberman, committed by rsmith).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.m
  clang/test/SemaCXX/attr-musttail.cpp

Index: clang/test/SemaCXX/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-musttail.cpp
@@ -0,0 +1,269 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();  // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;   // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return; // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return NoParams();  // expected-error {{cannot perform a tail call to function 'NoParams' because its signature is incompatible with the calling function}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]]  // expected-note {{tail call required by 'musttail' attribute here}}
+  return LongParam(x); // expected-error {{cannot perform a tail call to function 'LongParam' because its signature is incompatible with the calling function}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+  return ReturnsLong(); // expected-error {{cannot perform a tail call to function 'ReturnsLong' because its signature is incompatible with the calling function}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{'MemberFunction' declared here}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return st.MemberFunction(); // expected-error {{non-member function cannot perform a tail call to non-static member function 'MemberFunction'}}
+}
+
+void ReturnsVoid(); // expected-note {{'ReturnsVoid' declared here}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+return ReturnsVoid(); // expected-error{{non-static member function cannot perform a tail call to non-member function 'ReturnsVoid'}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsVoid(); // expected-error {{cannot perform a tail call from this return statement}}
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+int TestNonTrivialParam(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{tail call requires that the return value, all parameters, and any temporaries created by the expression 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added a comment.

Ok I think this is ready to land.

There are a few FIXME comments, I will follow up with some small changes to 
address them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 337597.
haberman added a comment.

- Fixed typo in comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.m
  clang/test/SemaCXX/attr-musttail.cpp

Index: clang/test/SemaCXX/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-musttail.cpp
@@ -0,0 +1,269 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();  // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;   // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return; // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return NoParams();  // expected-error {{cannot perform a tail call to function 'NoParams' because its signature is incompatible with the calling function}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]]  // expected-note {{tail call required by 'musttail' attribute here}}
+  return LongParam(x); // expected-error {{cannot perform a tail call to function 'LongParam' because its signature is incompatible with the calling function}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+  return ReturnsLong(); // expected-error {{cannot perform a tail call to function 'ReturnsLong' because its signature is incompatible with the calling function}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{'MemberFunction' declared here}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return st.MemberFunction(); // expected-error {{non-member function cannot perform a tail call to non-static member function 'MemberFunction'}}
+}
+
+void ReturnsVoid(); // expected-note {{'ReturnsVoid' declared here}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+return ReturnsVoid(); // expected-error{{non-static member function cannot perform a tail call to non-member function 'ReturnsVoid'}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsVoid(); // expected-error {{cannot perform a tail call from this return statement}}
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+int TestNonTrivialParam(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{tail call requires that the return value, all parameters, and any temporaries created by the expression are trivially destructible}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+HasNonTrivialDestructor TestReturnsNonTrivialValue() {
+  // FIXME: the diagnostic cannot 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Thanks, cool :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 337592.
haberman added a comment.

- Fixed several cases in CodeGen test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.m
  clang/test/SemaCXX/attr-musttail.cpp

Index: clang/test/SemaCXX/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-musttail.cpp
@@ -0,0 +1,269 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();  // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;   // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return; // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return NoParams();  // expected-error {{cannot perform a tail call to function 'NoParams' because its signature is incompatible with the calling function}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]]  // expected-note {{tail call required by 'musttail' attribute here}}
+  return LongParam(x); // expected-error {{cannot perform a tail call to function 'LongParam' because its signature is incompatible with the calling function}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+  return ReturnsLong(); // expected-error {{cannot perform a tail call to function 'ReturnsLong' because its signature is incompatible with the calling function}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{'MemberFunction' declared here}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return st.MemberFunction(); // expected-error {{non-member function cannot perform a tail call to non-static member function 'MemberFunction'}}
+}
+
+void ReturnsVoid(); // expected-note {{'ReturnsVoid' declared here}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+return ReturnsVoid(); // expected-error{{non-static member function cannot perform a tail call to non-member function 'ReturnsVoid'}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsVoid(); // expected-error {{cannot perform a tail call from this return statement}}
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+int TestNonTrivialParam(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{tail call requires that the return value, all parameters, and any temporaries created by the expression are trivially destructible}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+HasNonTrivialDestructor TestReturnsNonTrivialValue() {
+  // FIXME: the 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 337590.
haberman added a comment.

- Fixed release note escaping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.m
  clang/test/SemaCXX/attr-musttail.cpp

Index: clang/test/SemaCXX/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-musttail.cpp
@@ -0,0 +1,269 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();  // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;   // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return; // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return NoParams();  // expected-error {{cannot perform a tail call to function 'NoParams' because its signature is incompatible with the calling function}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]]  // expected-note {{tail call required by 'musttail' attribute here}}
+  return LongParam(x); // expected-error {{cannot perform a tail call to function 'LongParam' because its signature is incompatible with the calling function}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+  return ReturnsLong(); // expected-error {{cannot perform a tail call to function 'ReturnsLong' because its signature is incompatible with the calling function}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{'MemberFunction' declared here}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return st.MemberFunction(); // expected-error {{non-member function cannot perform a tail call to non-static member function 'MemberFunction'}}
+}
+
+void ReturnsVoid(); // expected-note {{'ReturnsVoid' declared here}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+return ReturnsVoid(); // expected-error{{non-static member function cannot perform a tail call to non-member function 'ReturnsVoid'}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsVoid(); // expected-error {{cannot perform a tail call from this return statement}}
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+int TestNonTrivialParam(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{tail call requires that the return value, all parameters, and any temporaries created by the expression are trivially destructible}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+HasNonTrivialDestructor TestReturnsNonTrivialValue() {
+  // FIXME: the diagnostic 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 337589.
haberman added a comment.

- Added release note for [[clang::musttail]].


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.m
  clang/test/SemaCXX/attr-musttail.cpp

Index: clang/test/SemaCXX/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-musttail.cpp
@@ -0,0 +1,269 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();  // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;   // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return; // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return NoParams();  // expected-error {{cannot perform a tail call to function 'NoParams' because its signature is incompatible with the calling function}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]]  // expected-note {{tail call required by 'musttail' attribute here}}
+  return LongParam(x); // expected-error {{cannot perform a tail call to function 'LongParam' because its signature is incompatible with the calling function}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+  return ReturnsLong(); // expected-error {{cannot perform a tail call to function 'ReturnsLong' because its signature is incompatible with the calling function}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{'MemberFunction' declared here}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return st.MemberFunction(); // expected-error {{non-member function cannot perform a tail call to non-static member function 'MemberFunction'}}
+}
+
+void ReturnsVoid(); // expected-note {{'ReturnsVoid' declared here}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+return ReturnsVoid(); // expected-error{{non-static member function cannot perform a tail call to non-member function 'ReturnsVoid'}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsVoid(); // expected-error {{cannot perform a tail call from this return statement}}
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+int TestNonTrivialParam(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{tail call requires that the return value, all parameters, and any temporaries created by the expression are trivially destructible}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+HasNonTrivialDestructor TestReturnsNonTrivialValue() {
+  // FIXME: the 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 337581.
haberman added a comment.

- More diagnostic wordsmithing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.m
  clang/test/SemaCXX/attr-musttail.cpp

Index: clang/test/SemaCXX/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-musttail.cpp
@@ -0,0 +1,269 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();  // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;   // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return; // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return NoParams();  // expected-error {{cannot perform a tail call to function 'NoParams' because its signature is incompatible with the calling function}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]]  // expected-note {{tail call required by 'musttail' attribute here}}
+  return LongParam(x); // expected-error {{cannot perform a tail call to function 'LongParam' because its signature is incompatible with the calling function}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+  return ReturnsLong(); // expected-error {{cannot perform a tail call to function 'ReturnsLong' because its signature is incompatible with the calling function}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{'MemberFunction' declared here}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return st.MemberFunction(); // expected-error {{non-member function cannot perform a tail call to non-static member function 'MemberFunction'}}
+}
+
+void ReturnsVoid(); // expected-note {{'ReturnsVoid' declared here}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+return ReturnsVoid(); // expected-error{{non-static member function cannot perform a tail call to non-member function 'ReturnsVoid'}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsVoid(); // expected-error {{cannot perform a tail call from this return statement}}
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+int TestNonTrivialParam(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{tail call requires that the return value, all parameters, and any temporaries created by the expression are trivially destructible}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+HasNonTrivialDestructor TestReturnsNonTrivialValue() {
+  // FIXME: the diagnostic cannot currently distinguish 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 337576.
haberman marked 6 inline comments as done.
haberman added a comment.

- Word-smithed diagnostics and addressed other review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.m
  clang/test/SemaCXX/attr-musttail.cpp

Index: clang/test/SemaCXX/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-musttail.cpp
@@ -0,0 +1,263 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();  // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;   // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return; // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return NoParams();  // expected-error {{tail call cannot invoke function 'NoParams' because its signature is incompatible with the calling function}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]]  // expected-note {{tail call required by 'musttail' attribute here}}
+  return LongParam(x); // expected-error {{tail call cannot invoke function 'LongParam' because its signature is incompatible with the calling function}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+  return ReturnsLong(); // expected-error {{tail call cannot invoke function 'ReturnsLong' because its signature is incompatible with the calling function}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{'MemberFunction' declared here}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] // expected-note {{tail call required by 'musttail' attribute here}}
+  return st.MemberFunction(); // expected-error {{tail call in non-member function cannot invoke non-static member function 'MemberFunction'}}
+}
+
+void ReturnsVoid(); // expected-note {{'ReturnsVoid' declared here}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]]   // expected-note {{tail call required by 'musttail' attribute here}}
+return ReturnsVoid(); // expected-error{{tail call in non-static member function cannot invoke non-member function 'ReturnsVoid'}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsVoid(); // expected-error {{cannot perform a tail call from this return statement}}
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+int TestNonTrivialParam(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{tail call requires that the return value, all parameters, and any temporaries created by the expression are trivially destructible}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+HasNonTrivialDestructor TestReturnsNonTrivialValue() {
+  // 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:711
+if (CalleeQualType.isNull()) {
+  // The function callee is invalid and already triggered an error.
+  // Avoid compounding errors.

haberman wrote:
> rsmith wrote:
> > Even in invalid code we should never see a `CallExpr` whose callee has a 
> > null type; if `Sema` can't form an `Expr` that meets the normal expression 
> > invariants during error recovery, it doesn't build one at all. I think you 
> > can remove this `if`.
> Without this if(), I crash on this test case. What do you think?
> 
> ```
> struct TestBadPMF {
>   int (TestBadPMF::*pmf)();
>   void BadPMF() {
> [[clang::musttail]] return ((*this)->*pmf)(); // expected-error {{left 
> hand operand to ->* must be a pointer to class compatible with the right hand 
> operand, but is 'TestBadPMF'}}
>   }
> };
> ```
> 
> Dump of `CalleeExpr` is:
> 
> ```
> RecoveryExpr 0x106671e8 '' contains-errors lvalue
> |-ParenExpr 0x10667020 'struct TestBadPMF' lvalue
> | `-UnaryOperator 0x10667008 'struct TestBadPMF' lvalue prefix '*' cannot 
> overflow
> |   `-CXXThisExpr 0x10666ff8 'struct TestBadPMF *' this
> `-MemberExpr 0x10667050 'int (struct TestBadPMF::*)(void)' lvalue ->pmf 
> 0x10666ed0
>   `-CXXThisExpr 0x10667040 'struct TestBadPMF *' implicit this
> ```
Ah, right, while the callee will always have a non-null type, that type might 
not be a pointer type.

I think what we're missing here is a check for a dependent callee; checking for 
a dependent context isn't enough to check for error-dependent constructs. 
Probably the simplest thing would be to change the `isDependentContext()` 
checks to also check if the return expression `isInstantiationDependent()`. 
(That would only help with the error-dependent cases for now, but we'd also 
need that extra check in the future if anything like 
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2277r0.html goes 
forward, allowing dependent constructs in non-dependent contexts, especially in 
combination with 
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1306r1.pdf.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:711
+if (CalleeQualType.isNull()) {
+  // The function callee is invalid and already triggered an error.
+  // Avoid compounding errors.

rsmith wrote:
> Even in invalid code we should never see a `CallExpr` whose callee has a null 
> type; if `Sema` can't form an `Expr` that meets the normal expression 
> invariants during error recovery, it doesn't build one at all. I think you 
> can remove this `if`.
Without this if(), I crash on this test case. What do you think?

```
struct TestBadPMF {
  int (TestBadPMF::*pmf)();
  void BadPMF() {
[[clang::musttail]] return ((*this)->*pmf)(); // expected-error {{left hand 
operand to ->* must be a pointer to class compatible with the right hand 
operand, but is 'TestBadPMF'}}
  }
};
```

Dump of `CalleeExpr` is:

```
RecoveryExpr 0x106671e8 '' contains-errors lvalue
|-ParenExpr 0x10667020 'struct TestBadPMF' lvalue
| `-UnaryOperator 0x10667008 'struct TestBadPMF' lvalue prefix '*' cannot 
overflow
|   `-CXXThisExpr 0x10666ff8 'struct TestBadPMF *' this
`-MemberExpr 0x10667050 'int (struct TestBadPMF::*)(void)' lvalue ->pmf 
0x10666ed0
  `-CXXThisExpr 0x10667040 'struct TestBadPMF *' implicit this
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Nice new feature! Please also update Release Notes for clang.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Functionally this looks good to me. I've suggested some minor cleanups and I 
understand you're doing some wordsmithing on the diagnostics; I think once 
those are complete this will be ready to land. Thank you!




Comment at: clang/lib/CodeGen/CGExpr.cpp:4828
  ReturnValueSlot ReturnValue) {
+  SaveAndRestore SaveMustTail(InMustTailCallExpr, E == MustTailCall);
+

haberman wrote:
> rsmith wrote:
> > The more I think about this, the more it makes me nervous: if any of the 
> > `Emit*CallExpr` functions below incidentally emit a call on the way to 
> > producing their results via the CGCall machinery, and do so without 
> > recursing through this function, that incidental call will be emitted as a 
> > tail call instead of the intended one. Specifically:
> > 
> > -- I could imagine a block call involving multiple function calls, 
> > depending on the blocks ABI.
> > -- I could imagine a member call performing a function call to convert from 
> > derived to virtual base in some ABIs.
> > -- A CUDA kernel call in general involves calling a setup function before 
> > the actual function call happens (and it doesn't make sense for a CUDA 
> > kernel call to be a tail call anyway...)
> > -- A call to a builtin can result in any number of function calls.
> > -- If any expression in the function arguments emits a call without calling 
> > back into this function, we'll emit that call as a tail call instead of 
> > this one. Eg, `[[clang::musttail]] return f(dynamic_cast(p));` might 
> > emit the call to `__cxa_dynamic_cast` as the tail call instead of emitting 
> > the call to `f` as the tail call, depending on whether the CGCall machinery 
> > is used when emitting the `__cxa_dynamic_cast` call.
> > 
> > Is it feasible to sink this check into the `CodeGenFunction::EmitCall` 
> > overload that takes a `CallExpr`, 
> > `CodeGenFunction::EmitCXXMemberOrOperatorCall`, and 
> > `CodeGenFunction::EmitCXXMemberPointerCallExpr`, after we've emitted the 
> > callee and call args? It looks like we might be able to check this 
> > immediately before calling the CGCall overload of `EmitCall`, so we could 
> > pass in the 'musttail' information as a flag or similar instead of using 
> > global state in the `CodeGenFunction` object; if so, it'd be much easier to 
> > be confident that we're applying the attribute to the right call.
> Done. It's feeling like `IsMustTail`, `callOrInvoke`, and `Loc` might want to 
> get collapsed into an options struct, especially given the default parameters 
> on the first two. Maybe could do as a follow up?
I agree, that sounds like a nice cleanup. Delaying this to a future change 
makes sense to me.



Comment at: clang/lib/Sema/SemaStmt.cpp:616
+
+  const auto *R = cast(St);
+  const Expr *Ex = R->getRetValue();

You shouldn't need the `const` in the argument to `cast`, and we generally omit 
it; `cast` copies the pointer/referenceness and qualifiers from its argument 
anyway, and the explicit `const` in the type of `R` seems sufficient for 
readers. (I'm not even sure if `cast` intends to permit explicit qualfiiers 
here.)



Comment at: clang/lib/Sema/SemaStmt.cpp:664
+  // Find caller function signature.
+  if (!CallerDecl || isa(CurContext)) {
+int ContextType;

I think this `isa` check is redundant, because a `CapturedDecl` 
is not a `FunctionDecl`, so `CallerDecl` will always be null when `CurContext` 
is a `CapturedDecl`.



Comment at: clang/lib/Sema/SemaStmt.cpp:711
+if (CalleeQualType.isNull()) {
+  // The function callee is invalid and already triggered an error.
+  // Avoid compounding errors.

Even in invalid code we should never see a `CallExpr` whose callee has a null 
type; if `Sema` can't form an `Expr` that meets the normal expression 
invariants during error recovery, it doesn't build one at all. I think you can 
remove this `if`.



Comment at: clang/lib/Sema/SemaStmt.cpp:771
+  if (!Context.hasSimilarType(A, B)) {
+PD << Select << A << B;
+return false;

Given that we don't care about differences in qualifiers, it might be clearer 
to not include them in the diagnostics.



Comment at: clang/test/CodeGenCXX/attr-musttail.cpp:212
+int TestNonCapturingLambda() {
+  auto lambda = []() { return 12; };   // expected-note {{target function is a 
member function of class 'const (lambda at 
/usr/local/google/home/haberman/code/llvm-project/clang/test/SemaCXX/attr-musttail.cpp:142:17)'}}
+  [[clang::musttail]] return (+lambda)();




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:605-609
+  while (!isa(Ex)) {
+auto *PE = cast(Ex);
+Ex = IgnoreImplicitAsWritten(PE->getSubExpr());
+PE->setSubExpr(Ex);
+  }

rsmith wrote:
> This loop is problematic: it's generally not safe to modify an expression 
> that is used as a subexpression of another expression. (Modifying the 
> `ReturnStmt` is, by contrast, much less problematic because the properties of 
> a statement have less complex dependencies on the properties of its 
> subexpressions.) In particular, if there were any implicit conversions here 
> that changed the type or value category or similar, the enclosing parentheses 
> would have the wrong type / value category / similar. Also there are 
> possibilities here other than `CallExpr` and `ParenExpr`, such as anything 
> else that we consider to be "parentheses" (such as a `GenericSelectionExpr`).
> 
> But I think this loop should never be necessary, because all implicit 
> conversions should always be on the outside of the parentheses. Do you have a 
> testcase that needs it?
I removed it and my test cases still pass. I'm glad to know this isn't 
necessary: I was coding defensively because I didn't know that I could count on 
this invariant:

> all implicit conversions should always be on the outside of the parentheses.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 337252.
haberman marked 14 inline comments as done.
haberman added a comment.

- Addressed more review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.m
  clang/test/SemaCXX/attr-musttail.cpp

Index: clang/test/SemaCXX/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-musttail.cpp
@@ -0,0 +1,227 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();  // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;   // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return; // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member function of class 'Struct1'}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{cannot 'musttail' from a non-member to a member function}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is not a member function}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{cannot 'musttail' from a member to a non-member function}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsVoid(); // expected-error {{cannot perform a tail call from this return statement}}
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+int TestNonTrivialParam(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{'musttail' attribute requires that the return value, all parameters, and any temporaries created by the expression are trivially destructible}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+HasNonTrivialDestructor TestReturnsNonTrivialValue() {
+  // FIXME: the diagnostic cannot currently distinguish between needing to run a
+  // destructor for the return value and needing to run a destructor for some
+  // other temporary created in the return statement.
+  [[clang::musttail]] return (ReturnsNonTrivialValue()); // expected-error {{'musttail' attribute requires that the return value, all parameters, and any temporaries created by the expression are trivially destructible}}
+}
+
+HasNonTrivialDestructor 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:603-604
+  ReturnStmt *R = cast(St);
+  R->setRetValue(IgnoreImplicitAsWritten(R->getRetValue()));
+  Expr *Ex = R->getRetValue();
+  while (!isa(Ex)) {

I think this would be clearer, assuming it's equivalent (and if it's not 
equivalent, I think it'd be useful to include a comment explaining why).



Comment at: clang/lib/Sema/SemaStmt.cpp:605-609
+  while (!isa(Ex)) {
+auto *PE = cast(Ex);
+Ex = IgnoreImplicitAsWritten(PE->getSubExpr());
+PE->setSubExpr(Ex);
+  }

This loop is problematic: it's generally not safe to modify an expression that 
is used as a subexpression of another expression. (Modifying the `ReturnStmt` 
is, by contrast, much less problematic because the properties of a statement 
have less complex dependencies on the properties of its subexpressions.) In 
particular, if there were any implicit conversions here that changed the type 
or value category or similar, the enclosing parentheses would have the wrong 
type / value category / similar. Also there are possibilities here other than 
`CallExpr` and `ParenExpr`, such as anything else that we consider to be 
"parentheses" (such as a `GenericSelectionExpr`).

But I think this loop should never be necessary, because all implicit 
conversions should always be on the outside of the parentheses. Do you have a 
testcase that needs it?



Comment at: clang/lib/Sema/SemaStmt.cpp:618
+
+  const ReturnStmt *R = cast(St);
+  const Expr *Ex = R->getRetValue();

... would be more in line with our normal idioms.



Comment at: clang/lib/Sema/SemaStmt.cpp:687
+// Caller is a non-method function.
+CallerType.Func = dyn_cast(CallerDecl->getType());
+  }

Use `getAs` rather than `dyn_cast` to look through type sugar. For example, in

```
void (f)() { [[clang::musttail]] return f(); }
```
... the type of `f` is a `ParenType`, not a `FunctionProtoType`.



Comment at: clang/lib/Sema/SemaStmt.cpp:697
+  return false;
+  } else if (VD && isa(VD->getType())) {
+// Call is: obj->*method_ptr or obj.*method_ptr

You need to use `getAs` here not `isa` in order to look 
through type sugar (eg, typedefs).

However, as noted above, a call via a member pointer doesn't necessarily have a 
`CalleeDecl`, so you'll need to do this check by looking for a callee 
expression that's the right kind of `BinaryOperator` instead.



Comment at: clang/lib/Sema/SemaStmt.cpp:636-637
+
+  if (!CE->getCalleeDecl()) {
+assert(hasUncompilableErrorOccurred() && "expected previous error");
+return false;

haberman wrote:
> aaron.ballman wrote:
> > This worries me slightly -- not all `CallExpr` objects have a callee 
> > declaration 
> > (https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1367).
> >  That said, I'm struggling to come up with an example that isn't covered so 
> > this may be fine.
> That was my experience too, I wasn't able to find a case that isn't covered. 
> I tried to avoid adding any diagnostics that I didn't know how to trigger or 
> test.
This assert is incorrect. It would fail for a case like:

```
using T = int();
T *f();
int g() { [[clang::musttail]] return f()(); }
```

... where there is no declaration associated with the function pointer returned 
by `f()`.

I think instead of looking for a callee declaration, you should instead inspect 
the callee expression. You can distinguish between a member function call and a 
non-member call by looking at the type of the callee. Perhaps the simplest way 
would be to distinguish between three cases:

(1) There is a callee declaration, which is a member function: this is a direct 
call to a member function; you can use the type of the callee declaration for 
your check.
(2) The callee expression is (after skipping parens) a pointer-to-member access 
operator (`BinaryOperator::isPtrMemOp`); you can use the type of the RHS 
operand (which will be a pointer to member function) for your check.
(3) Anything else: this is a non-member-function call, and you can directly 
inspect the type of the callee without caring about the callee declaration. 
(You might still find the type is not a function type at this stage, which 
indicates this is some kind of special form. In particular, it could be a 
`BuiltinType::BoundMember` for a pseudo-destructor call. I'm not sure if there 
are currently any other special cases that make it this far; there might not 
be, because most such cases are dependent.)



Comment at: clang/test/CodeGen/attr-musttail.cpp:1
+// RUN: %clang_cc1 -fno-elide-constructors -S -emit-llvm %s -triple 
x86_64-unknown-linux-gnu -o - | FileCheck %s
+// RUN: %clang_cc1 -fno-elide-constructors -S -emit-llvm %s -triple 
x86_64-unknown-linux-gnu -o - | opt -verify

This is 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman marked an inline comment as done.
haberman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:697-699
+  } else if (VD && dyn_cast(VD->getType())) {
+// Call is: obj->*method_ptr or obj.*method_ptr
+const auto *MPT = VD->getType()->castAs();

aaron.ballman wrote:
> I'm not certain if I should take a shower after writing that code or not, but 
> it's one potential way not to perform the cast twice.
> 
> If that code is too odious for others, we should at least change the 
> `dyn_cast<>` in the `else if` to be an `isa<>`.
I changed `dyn_cast<>` to `isa<>`. If @rsmith concurs about the 
`dyn_cast_or_null<>` variant I'll switch to that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 336894.
haberman added a comment.

- Switch to isa<> for type check.
- Merge branch 'main' into musttail


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp
  clang/test/Sema/attr-musttail.m

Index: clang/test/Sema/attr-musttail.m
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.m
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -Wno-objc-root-class -verify %s
+
+void TestObjcBlock(void) {
+  void (^x)(void) = ^(void) {
+__attribute__((musttail)) return TestObjcBlock(); // expected-error{{'musttail' attribute cannot be used from a block}}
+  };
+  __attribute__((musttail)) return x();
+}
+
+void ReturnsVoid(void);
+void TestObjcBlockVar(void) {
+  __block int i = 0;  // expected-note{{jump exits scope of __block variable}}
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{cannot perform a tail call from this return statement}}
+}
+
+__attribute__((objc_root_class))
+@interface TestObjcClass
+@end
+
+@implementation TestObjcClass
+
+- (void)testObjCMethod {
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{'musttail' attribute cannot be used from an Objective-C function}}
+}
+
+@end
Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,201 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();  // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;   // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return; // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member of different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is a member of different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:697-699
+  } else if (VD && dyn_cast(VD->getType())) {
+// Call is: obj->*method_ptr or obj.*method_ptr
+const auto *MPT = VD->getType()->castAs();

I'm not certain if I should take a shower after writing that code or not, but 
it's one potential way not to perform the cast twice.

If that code is too odious for others, we should at least change the 
`dyn_cast<>` in the `else if` to be an `isa<>`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:636-637
+
+  if (!CE->getCalleeDecl()) {
+assert(hasUncompilableErrorOccurred() && "expected previous error");
+return false;

aaron.ballman wrote:
> This worries me slightly -- not all `CallExpr` objects have a callee 
> declaration 
> (https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1367).
>  That said, I'm struggling to come up with an example that isn't covered so 
> this may be fine.
That was my experience too, I wasn't able to find a case that isn't covered. I 
tried to avoid adding any diagnostics that I didn't know how to trigger or test.



Comment at: clang/lib/Sema/SemaStmt.cpp:700
+// Call is: obj->*method_ptr or obj.*method_ptr
+const MemberPointerType *MPT = VD->getType()->castAs();
+CalleeType.This = QualType(MPT->getClass(), 0);

aaron.ballman wrote:
> It'd be better not to go through the cast machinery twice -- you cast to the 
> `MemberPointerType` and then cast to the same thing again (but in a different 
> way).
I changed to `auto`, but I can't tell if you have another suggestion here also. 
I can't see how any of these casts can be removed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 336511.
haberman marked 9 inline comments as done.
haberman added a comment.

- Simplified some casts and type declarations.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp
  clang/test/Sema/attr-musttail.m

Index: clang/test/Sema/attr-musttail.m
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.m
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -Wno-objc-root-class -verify %s
+
+void TestObjcBlock(void) {
+  void (^x)(void) = ^(void) {
+__attribute__((musttail)) return TestObjcBlock(); // expected-error{{'musttail' attribute cannot be used from a block}}
+  };
+  __attribute__((musttail)) return x();
+}
+
+void ReturnsVoid(void);
+void TestObjcBlockVar(void) {
+  __block int i = 0;  // expected-note{{jump exits scope of __block variable}}
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{cannot perform a tail call from this return statement}}
+}
+
+__attribute__((objc_root_class))
+@interface TestObjcClass
+@end
+
+@implementation TestObjcClass
+
+- (void)testObjCMethod {
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{'musttail' attribute cannot be used from an Objective-C function}}
+}
+
+@end
Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,201 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();  // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;   // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return; // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member of different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is a member of different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Mostly just nits from me, but the attribute portions look good to me.




Comment at: clang/include/clang/AST/IgnoreExpr.h:127
+  if (CCE && CCE->isElidable() && !isa(CCE)) {
+auto NumArgs = CCE->getNumArgs();
+if ((NumArgs == 1 ||





Comment at: clang/lib/Sema/SemaStmt.cpp:628
+
+  const CallExpr *CE =
+  dyn_cast_or_null(IgnoreParenImplicitAsWritten(Ex));





Comment at: clang/lib/Sema/SemaStmt.cpp:636-637
+
+  if (!CE->getCalleeDecl()) {
+assert(hasUncompilableErrorOccurred() && "expected previous error");
+return false;

This worries me slightly -- not all `CallExpr` objects have a callee 
declaration 
(https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1367). 
That said, I'm struggling to come up with an example that isn't covered so this 
may be fine.



Comment at: clang/lib/Sema/SemaStmt.cpp:641
+
+  if (const ExprWithCleanups *EWC = dyn_cast(Ex)) {
+if (EWC->cleanupsHaveSideEffects()) {





Comment at: clang/lib/Sema/SemaStmt.cpp:655
+
+  const FunctionDecl *CallerDecl = dyn_cast(CurContext);
+





Comment at: clang/lib/Sema/SemaStmt.cpp:659
+   bool IsCallee) -> bool {
+if (isa(CMD) || isa(CMD)) {
+  Diag(St->getBeginLoc(), diag::err_musttail_structors_forbidden) << 





Comment at: clang/lib/Sema/SemaStmt.cpp:682
+return false;
+  } else if (const CXXMethodDecl *CMD = dyn_cast(CurContext)) {
+// Caller is a class/struct method.





Comment at: clang/lib/Sema/SemaStmt.cpp:700
+// Call is: obj->*method_ptr or obj.*method_ptr
+const MemberPointerType *MPT = VD->getType()->castAs();
+CalleeType.This = QualType(MPT->getClass(), 0);

It'd be better not to go through the cast machinery twice -- you cast to the 
`MemberPointerType` and then cast to the same thing again (but in a different 
way).



Comment at: clang/lib/Sema/SemaStmtAttr.cpp:214
+SourceRange Range) {
+  MustTailAttr MTA(S.Context, A);
+

This can be removed entirely.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 336316.
haberman added a comment.

- Rename and refine IgnoreElidableImplicitConstructorSingleStep().


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp
  clang/test/Sema/attr-musttail.m

Index: clang/test/Sema/attr-musttail.m
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.m
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -Wno-objc-root-class -verify %s
+
+void TestObjcBlock(void) {
+  void (^x)(void) = ^(void) {
+__attribute__((musttail)) return TestObjcBlock(); // expected-error{{'musttail' attribute cannot be used from a block}}
+  };
+  __attribute__((musttail)) return x();
+}
+
+void ReturnsVoid(void);
+void TestObjcBlockVar(void) {
+  __block int i = 0;  // expected-note{{jump exits scope of __block variable}}
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{cannot perform a tail call from this return statement}}
+}
+
+__attribute__((objc_root_class))
+@interface TestObjcClass
+@end
+
+@implementation TestObjcClass
+
+- (void)testObjCMethod {
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{'musttail' attribute cannot be used from an Objective-C function}}
+}
+
+@end
Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,201 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();  // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;   // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return; // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member of different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is a member of different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2828-2829
+  "%0 attribute requires that the return value, all parameters, and any "
+  "temporaries created by the expression are trivially destructible and "
+  "do not require ARC">;
+def err_musttail_needs_call : Error<

rsmith wrote:
> Can we somehow avoid talking about ARC where it's not relevant? While it'd be 
> nice to be more precise here, my main concern is that we shouldn't be 
> mentioning ARC to people for whom it's not a meaningful term (eg, when not 
> compiling Objective-C or Objective-C++). Perhaps the simplest approach would 
> be to only mention ARC if `getLangOpts().ObjCAutoRefCount` is set?
I implemented this but I couldn't figure out how to actually trigger the ARC 
case, so I just removed that part of the diagnostic text for now.



Comment at: clang/lib/AST/AttrImpl.cpp:221-226
+  // Elide constructors even if this is disabled with -fno-elide-constructors
+  // because 'musttail' is a more local setting.
+  if (CCE && CCE->isElidable()) {
+assert(CCE->getNumArgs() == 1); // Copy or move constructor.
+Ex = CCE->getArg(0);
+  }

rsmith wrote:
> `IgnoreImplicitAsWritten` should already skip over implicit elidable 
> constructors, so I would imagine this is skipping over elidable //explicit// 
> constructor calls (eg, `[[musttail]] return T(make());` would perform a 
> tail-call to `make()`). Is that what we want?
As discussed offline, it appears that `IgnoreImplicitAsWritten()` was not 
skipping the implicit constructor in this case. Per our discussion, I created a 
new version of `IgnoreImplicitAsWritten()` that does, with a FIXME to land it 
in `Expr`, and I made it skip implicit constructors only (and added tests for 
this case).



Comment at: clang/lib/CodeGen/CGStmt.cpp:665
+  SaveAndRestore save_musttail(MustTailCall, musttail);
   EmitStmt(S.getSubStmt(), S.getAttrs());
 }

rsmith wrote:
> In the case where we're forcibly eliding a constructor, we'll need to emit a 
> return statement that returns `musttail` call expression here rather than 
> emitting the original substatement. Otherwise the tail call we emit will be 
> initializing a local temporary rather than initializing our return slot. Eg, 
> given:
> 
> ```
> struct A {
>   A(const A&);
>   ~A();
>   char data[32];
> };
> A f();
> A g() {
>   [[clang::musttail]] return f();
> }
> ```
> under `-fno-elide-constructors` when targeting C++11, say, we'll normally 
> lower that into something like:
> ```
> void f(A *return_slot);
> void g(A *return_slot) {
>   A temporary; //uninitialized
>   f(); // call f
>   A::A(return_slot, temporary); // call copy constructor to copy into return 
> slot
> }
> ```
> ... and with the current patch, it looks like we'll add a 'ret void' after 
> the call to `f`, leaving `g`'s return slot uninitialized and passing an 
> address into `f` that refers to a variable that will no longer exist once `f` 
> is called. We need to instead lower to:
> ```
> void f(A *return_slot);
> void g(A *return_slot) {
>   f(return_slot); // call f
> }
> ```
> Probably the easiest way to do this would be to change the return value on 
> the `ReturnStmt` to be the tail-called `CallExpr` when attaching the 
> attribute.
Done.

I had to change your test case to remove the destructor, otherwise it fails the 
trivial destruction check.

Take a look at the CodeGen tests and see if the output looks correct to you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 336310.
haberman marked 3 inline comments as done.
haberman added a comment.

- Refined the implicit constructor skipping code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/AST/IgnoreExpr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp
  clang/test/Sema/attr-musttail.m

Index: clang/test/Sema/attr-musttail.m
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.m
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -Wno-objc-root-class -verify %s
+
+void TestObjcBlock(void) {
+  void (^x)(void) = ^(void) {
+__attribute__((musttail)) return TestObjcBlock(); // expected-error{{'musttail' attribute cannot be used from a block}}
+  };
+  __attribute__((musttail)) return x();
+}
+
+void ReturnsVoid(void);
+void TestObjcBlockVar(void) {
+  __block int i = 0;  // expected-note{{jump exits scope of __block variable}}
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{cannot perform a tail call from this return statement}}
+}
+
+__attribute__((objc_root_class))
+@interface TestObjcClass
+@end
+
+@implementation TestObjcClass
+
+- (void)testObjCMethod {
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{'musttail' attribute cannot be used from an Objective-C function}}
+}
+
+@end
Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,201 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();  // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;   // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return; // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member of different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is a member of different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2828-2829
+  "%0 attribute requires that the return value, all parameters, and any "
+  "temporaries created by the expression are trivially destructible and "
+  "do not require ARC">;
+def err_musttail_needs_call : Error<

Can we somehow avoid talking about ARC where it's not relevant? While it'd be 
nice to be more precise here, my main concern is that we shouldn't be 
mentioning ARC to people for whom it's not a meaningful term (eg, when not 
compiling Objective-C or Objective-C++). Perhaps the simplest approach would be 
to only mention ARC if `getLangOpts().ObjCAutoRefCount` is set?



Comment at: clang/lib/AST/AttrImpl.cpp:221-226
+  // Elide constructors even if this is disabled with -fno-elide-constructors
+  // because 'musttail' is a more local setting.
+  if (CCE && CCE->isElidable()) {
+assert(CCE->getNumArgs() == 1); // Copy or move constructor.
+Ex = CCE->getArg(0);
+  }

`IgnoreImplicitAsWritten` should already skip over implicit elidable 
constructors, so I would imagine this is skipping over elidable //explicit// 
constructor calls (eg, `[[musttail]] return T(make());` would perform a 
tail-call to `make()`). Is that what we want?



Comment at: clang/lib/CodeGen/CGStmt.cpp:665
+  SaveAndRestore save_musttail(MustTailCall, musttail);
   EmitStmt(S.getSubStmt(), S.getAttrs());
 }

In the case where we're forcibly eliding a constructor, we'll need to emit a 
return statement that returns `musttail` call expression here rather than 
emitting the original substatement. Otherwise the tail call we emit will be 
initializing a local temporary rather than initializing our return slot. Eg, 
given:

```
struct A {
  A(const A&);
  ~A();
  char data[32];
};
A f();
A g() {
  [[clang::musttail]] return f();
}
```
under `-fno-elide-constructors` when targeting C++11, say, we'll normally lower 
that into something like:
```
void f(A *return_slot);
void g(A *return_slot) {
  A temporary; //uninitialized
  f(); // call f
  A::A(return_slot, temporary); // call copy constructor to copy into return 
slot
}
```
... and with the current patch, it looks like we'll add a 'ret void' after the 
call to `f`, leaving `g`'s return slot uninitialized and passing an address 
into `f` that refers to a variable that will no longer exist once `f` is 
called. We need to instead lower to:
```
void f(A *return_slot);
void g(A *return_slot) {
  f(return_slot); // call f
}
```
Probably the easiest way to do this would be to change the return value on the 
`ReturnStmt` to be the tail-called `CallExpr` when attaching the attribute.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman marked 2 inline comments as done.
haberman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

aaron.ballman wrote:
> haberman wrote:
> > aaron.ballman wrote:
> > > haberman wrote:
> > > > aaron.ballman wrote:
> > > > > haberman wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > rsmith wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > haberman wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > haberman wrote:
> > > > > > > > > > > > > haberman wrote:
> > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > haberman wrote:
> > > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > > This functionality belongs in 
> > > > > > > > > > > > > > > > > SemaStmtAttr.cpp, I think.
> > > > > > > > > > > > > > > > That is where I had originally put it, but that 
> > > > > > > > > > > > > > > > didn't work for templates. The semantic checks 
> > > > > > > > > > > > > > > > can only be performed at instantiation time. 
> > > > > > > > > > > > > > > > `ActOnAttributedStmt` seems to be the right 
> > > > > > > > > > > > > > > > hook point where I can evaluate the semantic 
> > > > > > > > > > > > > > > > checks for both template and non-template 
> > > > > > > > > > > > > > > > functions (with template functions getting 
> > > > > > > > > > > > > > > > checked at instantiation time).
> > > > > > > > > > > > > > > I disagree that `ActOnAttributedStmt()` is the 
> > > > > > > > > > > > > > > correct place for this checking -- template 
> > > > > > > > > > > > > > > checking should occur when the template is 
> > > > > > > > > > > > > > > instantiated, same as happens for declaration 
> > > > > > > > > > > > > > > attributes. I'd like to see this functionality 
> > > > > > > > > > > > > > > moved to SemaStmtAttr.cpp. Keeping the attribute 
> > > > > > > > > > > > > > > logic together and following the same patterns is 
> > > > > > > > > > > > > > > what allows us to tablegenerate more of the 
> > > > > > > > > > > > > > > attribute logic. Statement attributes are just 
> > > > > > > > > > > > > > > starting to get more such automation.
> > > > > > > > > > > > > > I tried commenting out this code and adding the 
> > > > > > > > > > > > > > following code into `handleMustTailAttr()` in 
> > > > > > > > > > > > > > `SemaStmtAttr.cpp`:
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > ```
> > > > > > > > > > > > > >   if (!S.checkMustTailAttr(St, MTA))
> > > > > > > > > > > > > > return nullptr;
> > > > > > > > > > > > > > ```
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > This caused my test cases related to templates to 
> > > > > > > > > > > > > > fail. It also seemed to break test cases related to 
> > > > > > > > > > > > > > `JumpDiagnostics`. My interpretation of this is 
> > > > > > > > > > > > > > that `handleMustTailAttr()` is called during 
> > > > > > > > > > > > > > parsing only, and cannot catch errors at template 
> > > > > > > > > > > > > > instantiation time or that require a more complete 
> > > > > > > > > > > > > > AST.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > What am I missing? Where in SemaStmtAttr.cpp are 
> > > > > > > > > > > > > > you suggesting that I put this check?
> > > > > > > > > > > > > Scratch the part about `JumpDiagnostics`, that was me 
> > > > > > > > > > > > > failing to call `S.setFunctionHasMustTail()`. I added 
> > > > > > > > > > > > > that and now the `JumpDiagnostics` tests pass.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > But the template test cases still fail, and I can't 
> > > > > > > > > > > > > find any hook point in `SemaStmtAttr.cpp` that will 
> > > > > > > > > > > > > let me evaluate these checks at template 
> > > > > > > > > > > > > instantiation time.
> > > > > > > > > > > > I think there's a bit of an architectural mixup, but 
> > > > > > > > > > > > I'm curious if @rsmith agrees before anyone starts 
> > > > > > > > > > > > doing work to make changes.
> > > > > > > > > > > > 
> > > > > > > > > > > > When transforming declarations, `RebuildWhatever()` 
> > > > > > > > > > > > calls the `ActOnWhatever()` function which calls 
> > > > > > > > > > > > `ProcessDeclAttributeList()` so that attributes are 
> > > > > > > > > > > > processed. `RebuildAttributedStmt()` similarly calls 
> > > > > > > > > > > > `ActOnAttributedStmt()`. However, 
> > > > > > > > > > > > `ActOnAttributedStmt()` doesn't call 
> > > > > > > > > > > > `ProcessStmtAttributes()` -- the logic is reversed so 
> > > > > > > > > > > > that `ProcessStmtAttributes()` is what calls 
> > > > > > > > > > > > `ActOnAttributedStmt()`.
> > > > > > > > > > > > 
> > > > > > > > > > > > I think 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 336203.
haberman added a comment.

- Formatted files with clang-format.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/AttrImpl.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp
  clang/test/Sema/attr-musttail.m

Index: clang/test/Sema/attr-musttail.m
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.m
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -Wno-objc-root-class -verify %s
+
+void TestObjcBlock(void) {
+  void (^x)(void) = ^(void) {
+__attribute__((musttail)) return TestObjcBlock(); // expected-error{{'musttail' attribute cannot be used from a block}}
+  };
+  __attribute__((musttail)) return x();
+}
+
+void ReturnsVoid(void);
+void TestObjcBlockVar(void) {
+  __block int i = 0;  // expected-note{{jump exits scope of __block variable}}
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{cannot perform a tail call from this return statement}}
+}
+
+//#import 
+
+__attribute__((objc_root_class))
+@interface TestObjcClass
+@end
+
+@implementation TestObjcClass
+
+- (void)testObjCMethod {
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{'musttail' attribute cannot be used from an Objective-C function}}
+}
+
+@end
Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,191 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();  // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;   // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return; // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member of different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is a member of different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added a comment.

In D99517#2667025 , @rjmccall wrote:

> You should structure this code so it's easy to add exceptions for certain 
> calling conventions that can support tail calls with weaker restrictions 
> (principally, callee-pop conventions).  Mostly that probably means checking 
> the calling convention first, or extracting the type restriction checks into 
> a different function that you can skip.  For example, I believe x86's 
> `fastcall` convention can logically support any combination of prototypes as 
> `musttail` as long as the return types are vaguely compatible.

I moved the calling convention check to be as early as possible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 336153.
haberman added a comment.

- Moved calling convention check to happen as early as possible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/AttrImpl.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp
  clang/test/Sema/attr-musttail.m

Index: clang/test/Sema/attr-musttail.m
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.m
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -Wno-objc-root-class -verify %s
+
+void TestObjcBlock(void) {
+  void (^x)(void) = ^(void) {
+__attribute__((musttail)) return TestObjcBlock(); // expected-error{{'musttail' attribute cannot be used from a block}}
+  };
+  __attribute__((musttail)) return x();
+}
+
+void ReturnsVoid(void);
+void TestObjcBlockVar(void) {
+  __block int i = 0;  // expected-note{{jump exits scope of __block variable}}
+  __attribute__((musttail)) return ReturnsVoid();  // expected-error{{cannot perform a tail call from this return statement}}
+}
+
+//#import 
+
+__attribute__((objc_root_class))
+@interface TestObjcClass
+@end
+
+@implementation TestObjcClass
+
+- (void)testObjCMethod {
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{'musttail' attribute cannot be used from an Objective-C function}}
+}
+
+@end
Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,191 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();   // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;// expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return;  // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member of different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is a member of different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

haberman wrote:
> aaron.ballman wrote:
> > haberman wrote:
> > > aaron.ballman wrote:
> > > > haberman wrote:
> > > > > aaron.ballman wrote:
> > > > > > rsmith wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > haberman wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > haberman wrote:
> > > > > > > > > > > > haberman wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > haberman wrote:
> > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > This functionality belongs in SemaStmtAttr.cpp, 
> > > > > > > > > > > > > > > > I think.
> > > > > > > > > > > > > > > That is where I had originally put it, but that 
> > > > > > > > > > > > > > > didn't work for templates. The semantic checks 
> > > > > > > > > > > > > > > can only be performed at instantiation time. 
> > > > > > > > > > > > > > > `ActOnAttributedStmt` seems to be the right hook 
> > > > > > > > > > > > > > > point where I can evaluate the semantic checks 
> > > > > > > > > > > > > > > for both template and non-template functions 
> > > > > > > > > > > > > > > (with template functions getting checked at 
> > > > > > > > > > > > > > > instantiation time).
> > > > > > > > > > > > > > I disagree that `ActOnAttributedStmt()` is the 
> > > > > > > > > > > > > > correct place for this checking -- template 
> > > > > > > > > > > > > > checking should occur when the template is 
> > > > > > > > > > > > > > instantiated, same as happens for declaration 
> > > > > > > > > > > > > > attributes. I'd like to see this functionality 
> > > > > > > > > > > > > > moved to SemaStmtAttr.cpp. Keeping the attribute 
> > > > > > > > > > > > > > logic together and following the same patterns is 
> > > > > > > > > > > > > > what allows us to tablegenerate more of the 
> > > > > > > > > > > > > > attribute logic. Statement attributes are just 
> > > > > > > > > > > > > > starting to get more such automation.
> > > > > > > > > > > > > I tried commenting out this code and adding the 
> > > > > > > > > > > > > following code into `handleMustTailAttr()` in 
> > > > > > > > > > > > > `SemaStmtAttr.cpp`:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > ```
> > > > > > > > > > > > >   if (!S.checkMustTailAttr(St, MTA))
> > > > > > > > > > > > > return nullptr;
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > 
> > > > > > > > > > > > > This caused my test cases related to templates to 
> > > > > > > > > > > > > fail. It also seemed to break test cases related to 
> > > > > > > > > > > > > `JumpDiagnostics`. My interpretation of this is that 
> > > > > > > > > > > > > `handleMustTailAttr()` is called during parsing only, 
> > > > > > > > > > > > > and cannot catch errors at template instantiation 
> > > > > > > > > > > > > time or that require a more complete AST.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > What am I missing? Where in SemaStmtAttr.cpp are you 
> > > > > > > > > > > > > suggesting that I put this check?
> > > > > > > > > > > > Scratch the part about `JumpDiagnostics`, that was me 
> > > > > > > > > > > > failing to call `S.setFunctionHasMustTail()`. I added 
> > > > > > > > > > > > that and now the `JumpDiagnostics` tests pass.
> > > > > > > > > > > > 
> > > > > > > > > > > > But the template test cases still fail, and I can't 
> > > > > > > > > > > > find any hook point in `SemaStmtAttr.cpp` that will let 
> > > > > > > > > > > > me evaluate these checks at template instantiation time.
> > > > > > > > > > > I think there's a bit of an architectural mixup, but I'm 
> > > > > > > > > > > curious if @rsmith agrees before anyone starts doing work 
> > > > > > > > > > > to make changes.
> > > > > > > > > > > 
> > > > > > > > > > > When transforming declarations, `RebuildWhatever()` calls 
> > > > > > > > > > > the `ActOnWhatever()` function which calls 
> > > > > > > > > > > `ProcessDeclAttributeList()` so that attributes are 
> > > > > > > > > > > processed. `RebuildAttributedStmt()` similarly calls 
> > > > > > > > > > > `ActOnAttributedStmt()`. However, `ActOnAttributedStmt()` 
> > > > > > > > > > > doesn't call `ProcessStmtAttributes()` -- the logic is 
> > > > > > > > > > > reversed so that `ProcessStmtAttributes()` is what calls 
> > > > > > > > > > > `ActOnAttributedStmt()`.
> > > > > > > > > > > 
> > > > > > > > > > > I think the correct answer is to switch the logic so that 
> > > > > > > > > > > `ActOnAttributedStmt()` calls `ProcessStmtAttributes()`, 
> > > > > > > > > > > then the template logic should automatically work.
> > > > > > > > > > > I think the correct answer is to switch the logic so that 
> > 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

aaron.ballman wrote:
> haberman wrote:
> > aaron.ballman wrote:
> > > haberman wrote:
> > > > aaron.ballman wrote:
> > > > > rsmith wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > haberman wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > haberman wrote:
> > > > > > > > > > > haberman wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > haberman wrote:
> > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > This functionality belongs in SemaStmtAttr.cpp, I 
> > > > > > > > > > > > > > > think.
> > > > > > > > > > > > > > That is where I had originally put it, but that 
> > > > > > > > > > > > > > didn't work for templates. The semantic checks can 
> > > > > > > > > > > > > > only be performed at instantiation time. 
> > > > > > > > > > > > > > `ActOnAttributedStmt` seems to be the right hook 
> > > > > > > > > > > > > > point where I can evaluate the semantic checks for 
> > > > > > > > > > > > > > both template and non-template functions (with 
> > > > > > > > > > > > > > template functions getting checked at instantiation 
> > > > > > > > > > > > > > time).
> > > > > > > > > > > > > I disagree that `ActOnAttributedStmt()` is the 
> > > > > > > > > > > > > correct place for this checking -- template checking 
> > > > > > > > > > > > > should occur when the template is instantiated, same 
> > > > > > > > > > > > > as happens for declaration attributes. I'd like to 
> > > > > > > > > > > > > see this functionality moved to SemaStmtAttr.cpp. 
> > > > > > > > > > > > > Keeping the attribute logic together and following 
> > > > > > > > > > > > > the same patterns is what allows us to tablegenerate 
> > > > > > > > > > > > > more of the attribute logic. Statement attributes are 
> > > > > > > > > > > > > just starting to get more such automation.
> > > > > > > > > > > > I tried commenting out this code and adding the 
> > > > > > > > > > > > following code into `handleMustTailAttr()` in 
> > > > > > > > > > > > `SemaStmtAttr.cpp`:
> > > > > > > > > > > > 
> > > > > > > > > > > > ```
> > > > > > > > > > > >   if (!S.checkMustTailAttr(St, MTA))
> > > > > > > > > > > > return nullptr;
> > > > > > > > > > > > ```
> > > > > > > > > > > > 
> > > > > > > > > > > > This caused my test cases related to templates to fail. 
> > > > > > > > > > > > It also seemed to break test cases related to 
> > > > > > > > > > > > `JumpDiagnostics`. My interpretation of this is that 
> > > > > > > > > > > > `handleMustTailAttr()` is called during parsing only, 
> > > > > > > > > > > > and cannot catch errors at template instantiation time 
> > > > > > > > > > > > or that require a more complete AST.
> > > > > > > > > > > > 
> > > > > > > > > > > > What am I missing? Where in SemaStmtAttr.cpp are you 
> > > > > > > > > > > > suggesting that I put this check?
> > > > > > > > > > > Scratch the part about `JumpDiagnostics`, that was me 
> > > > > > > > > > > failing to call `S.setFunctionHasMustTail()`. I added 
> > > > > > > > > > > that and now the `JumpDiagnostics` tests pass.
> > > > > > > > > > > 
> > > > > > > > > > > But the template test cases still fail, and I can't find 
> > > > > > > > > > > any hook point in `SemaStmtAttr.cpp` that will let me 
> > > > > > > > > > > evaluate these checks at template instantiation time.
> > > > > > > > > > I think there's a bit of an architectural mixup, but I'm 
> > > > > > > > > > curious if @rsmith agrees before anyone starts doing work 
> > > > > > > > > > to make changes.
> > > > > > > > > > 
> > > > > > > > > > When transforming declarations, `RebuildWhatever()` calls 
> > > > > > > > > > the `ActOnWhatever()` function which calls 
> > > > > > > > > > `ProcessDeclAttributeList()` so that attributes are 
> > > > > > > > > > processed. `RebuildAttributedStmt()` similarly calls 
> > > > > > > > > > `ActOnAttributedStmt()`. However, `ActOnAttributedStmt()` 
> > > > > > > > > > doesn't call `ProcessStmtAttributes()` -- the logic is 
> > > > > > > > > > reversed so that `ProcessStmtAttributes()` is what calls 
> > > > > > > > > > `ActOnAttributedStmt()`.
> > > > > > > > > > 
> > > > > > > > > > I think the correct answer is to switch the logic so that 
> > > > > > > > > > `ActOnAttributedStmt()` calls `ProcessStmtAttributes()`, 
> > > > > > > > > > then the template logic should automatically work.
> > > > > > > > > > I think the correct answer is to switch the logic so that 
> > > > > > > > > > ActOnAttributedStmt() calls ProcessStmtAttributes()
> > > > > > > > > 
> > > > > > > > > I think this would require `ProcessStmtAttributes()` to be 
> > > > > > > > > split into two 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 336141.
haberman added a comment.

- Factored duplicated code into a method on MustTailAttr.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/AttrImpl.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp
  clang/test/Sema/attr-musttail.m

Index: clang/test/Sema/attr-musttail.m
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.m
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -Wno-objc-root-class -verify %s
+
+void TestObjcBlock(void) {
+  void (^x)(void) = ^(void) {
+__attribute__((musttail)) return TestObjcBlock(); // expected-error{{'musttail' attribute cannot be used from a block}}
+  };
+  __attribute__((musttail)) return x();
+}
+
+void ReturnsVoid(void);
+void TestObjcBlockVar(void) {
+  __block int i = 0;  // expected-note{{jump exits scope of __block variable}}
+  __attribute__((musttail)) return ReturnsVoid();  // expected-error{{cannot perform a tail call from this return statement}}
+}
+
+//#import 
+
+__attribute__((objc_root_class))
+@interface TestObjcClass
+@end
+
+@implementation TestObjcClass
+
+- (void)testObjCMethod {
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{'musttail' attribute cannot be used from an Objective-C function}}
+}
+
+@end
Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,191 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();   // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;// expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return;  // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member of different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is a member of different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 336130.
haberman added a comment.

- Added FIXME for attribute refactoring.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp
  clang/test/Sema/attr-musttail.m

Index: clang/test/Sema/attr-musttail.m
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.m
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -Wno-objc-root-class -verify %s
+
+void TestObjcBlock(void) {
+  void (^x)(void) = ^(void) {
+__attribute__((musttail)) return TestObjcBlock(); // expected-error{{'musttail' attribute cannot be used from a block}}
+  };
+  __attribute__((musttail)) return x();
+}
+
+void ReturnsVoid(void);
+void TestObjcBlockVar(void) {
+  __block int i = 0;  // expected-note{{jump exits scope of __block variable}}
+  __attribute__((musttail)) return ReturnsVoid();  // expected-error{{cannot perform a tail call from this return statement}}
+}
+
+//#import 
+
+__attribute__((objc_root_class))
+@interface TestObjcClass
+@end
+
+@implementation TestObjcClass
+
+- (void)testObjCMethod {
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{'musttail' attribute cannot be used from an Objective-C function}}
+}
+
+@end
Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,191 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();   // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;// expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return;  // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member of different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is a member of different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo; 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

haberman wrote:
> aaron.ballman wrote:
> > haberman wrote:
> > > aaron.ballman wrote:
> > > > rsmith wrote:
> > > > > aaron.ballman wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > haberman wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > haberman wrote:
> > > > > > > > > > haberman wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > haberman wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > This functionality belongs in SemaStmtAttr.cpp, I 
> > > > > > > > > > > > > > think.
> > > > > > > > > > > > > That is where I had originally put it, but that 
> > > > > > > > > > > > > didn't work for templates. The semantic checks can 
> > > > > > > > > > > > > only be performed at instantiation time. 
> > > > > > > > > > > > > `ActOnAttributedStmt` seems to be the right hook 
> > > > > > > > > > > > > point where I can evaluate the semantic checks for 
> > > > > > > > > > > > > both template and non-template functions (with 
> > > > > > > > > > > > > template functions getting checked at instantiation 
> > > > > > > > > > > > > time).
> > > > > > > > > > > > I disagree that `ActOnAttributedStmt()` is the correct 
> > > > > > > > > > > > place for this checking -- template checking should 
> > > > > > > > > > > > occur when the template is instantiated, same as 
> > > > > > > > > > > > happens for declaration attributes. I'd like to see 
> > > > > > > > > > > > this functionality moved to SemaStmtAttr.cpp. Keeping 
> > > > > > > > > > > > the attribute logic together and following the same 
> > > > > > > > > > > > patterns is what allows us to tablegenerate more of the 
> > > > > > > > > > > > attribute logic. Statement attributes are just starting 
> > > > > > > > > > > > to get more such automation.
> > > > > > > > > > > I tried commenting out this code and adding the following 
> > > > > > > > > > > code into `handleMustTailAttr()` in `SemaStmtAttr.cpp`:
> > > > > > > > > > > 
> > > > > > > > > > > ```
> > > > > > > > > > >   if (!S.checkMustTailAttr(St, MTA))
> > > > > > > > > > > return nullptr;
> > > > > > > > > > > ```
> > > > > > > > > > > 
> > > > > > > > > > > This caused my test cases related to templates to fail. 
> > > > > > > > > > > It also seemed to break test cases related to 
> > > > > > > > > > > `JumpDiagnostics`. My interpretation of this is that 
> > > > > > > > > > > `handleMustTailAttr()` is called during parsing only, and 
> > > > > > > > > > > cannot catch errors at template instantiation time or 
> > > > > > > > > > > that require a more complete AST.
> > > > > > > > > > > 
> > > > > > > > > > > What am I missing? Where in SemaStmtAttr.cpp are you 
> > > > > > > > > > > suggesting that I put this check?
> > > > > > > > > > Scratch the part about `JumpDiagnostics`, that was me 
> > > > > > > > > > failing to call `S.setFunctionHasMustTail()`. I added that 
> > > > > > > > > > and now the `JumpDiagnostics` tests pass.
> > > > > > > > > > 
> > > > > > > > > > But the template test cases still fail, and I can't find 
> > > > > > > > > > any hook point in `SemaStmtAttr.cpp` that will let me 
> > > > > > > > > > evaluate these checks at template instantiation time.
> > > > > > > > > I think there's a bit of an architectural mixup, but I'm 
> > > > > > > > > curious if @rsmith agrees before anyone starts doing work to 
> > > > > > > > > make changes.
> > > > > > > > > 
> > > > > > > > > When transforming declarations, `RebuildWhatever()` calls the 
> > > > > > > > > `ActOnWhatever()` function which calls 
> > > > > > > > > `ProcessDeclAttributeList()` so that attributes are 
> > > > > > > > > processed. `RebuildAttributedStmt()` similarly calls 
> > > > > > > > > `ActOnAttributedStmt()`. However, `ActOnAttributedStmt()` 
> > > > > > > > > doesn't call `ProcessStmtAttributes()` -- the logic is 
> > > > > > > > > reversed so that `ProcessStmtAttributes()` is what calls 
> > > > > > > > > `ActOnAttributedStmt()`.
> > > > > > > > > 
> > > > > > > > > I think the correct answer is to switch the logic so that 
> > > > > > > > > `ActOnAttributedStmt()` calls `ProcessStmtAttributes()`, then 
> > > > > > > > > the template logic should automatically work.
> > > > > > > > > I think the correct answer is to switch the logic so that 
> > > > > > > > > ActOnAttributedStmt() calls ProcessStmtAttributes()
> > > > > > > > 
> > > > > > > > I think this would require `ProcessStmtAttributes()` to be 
> > > > > > > > split into two separate functions. Currently that function is 
> > > > > > > > doing two separate things:
> > > > > > > > 
> > > > > > > > 1. Translation of `ParsedAttr` into various subclasses of 
> > > > > > > > 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:458
+same number of arguments as the caller. The types of the return value and all
+arguments must be similar, including the implicit "this" argument, if any.
+Any variables in scope, including all arguments to the function and the

aaron.ballman wrote:
> It'd be nice if we could nail down "similar" somewhat. I don't know if `int` 
> and `short` are similar (due to promotions) or `const int` and `int` are 
> similar, etc.
Done. I tried to summarize the C++ concept of "similar" types as defined in 
https://eel.is/c++draft/conv.qual#2 and implemented in 
https://clang.llvm.org/doxygen/classclang_1_1ASTContext.html#a1b1b3b7a67a30fd817ba85454780d8ad



Comment at: clang/lib/Sema/SemaStmt.cpp:596
+  if (!CE->getCalleeDecl()) {
+assert(hasUncompilableErrorOccurred() && "expected previous error");
+return false;

rsmith wrote:
> A call expression doesn't necessarily have a known callee declaration. I 
> would expect this assert to fire on a case like:
> ```
> void f() {
>   void (*p)() = f;
>   [[clang::musttail]] return p();
> }
> ```
> We should reject this with a diagnostic.
I think this case will work actually, the callee decl in this case is just the 
function pointer, which seems appropriate and type checks correctly.

I added a test for this.



Comment at: clang/lib/Sema/SemaStmt.cpp:631
+// Caller is an Obj-C block decl: ^(void) { /* ... */ }
+assert(dyn_cast(CurContext) && "unexpected decl context");
+Diag(St->getBeginLoc(), diag::err_musttail_from_block_forbidden) << 

rjmccall wrote:
> rsmith wrote:
> > There are a couple of other contexts that can include a return statement: 
> > the caller could also be an `ObjCMethodDecl` (an Objective-C method) or a 
> > `CapturedDecl` (the body of a `#pragma omp` parallel region). I'd probably 
> > use a specific diagnostic ("cannot be used from a block" / "cannot be used 
> > from an Objective-C function") for the block and ObjCMethod case, and a 
> > nonsepcific-but-correct "cannot be used from this context" for anything 
> > else.
> Blocks ought to be extremely straightforward to support.  Just validate that 
> the tail call is to a block pointer and then compare the underlying function 
> types line up in the same way.  You will need to be able to verify that there 
> isn't a non-trivial conversion on the return types, even if the return type 
> isn't known at this point in the function, but that's a problem in C++ as 
> well due to lambdas and `auto` deduced return types.
> 
> Also, you can use `isa<...>` for checks like this instead of `dyn_cast<...>`.
Tail calls to a block are indeed straightforward and are handled below. This 
check is for tail calls from a block, which I tried to add support for but 
didn't have much luck (in particular, during parsing of a block I wasn't able 
to get good type information for the block).

> I'd probably use a specific diagnostic ("cannot be used from a block" / 
> "cannot be used from an Objective-C function") for the block and ObjCMethod 
> case, and a nonsepcific-but-correct "cannot be used from this context" for 
> anything else.

I implemented this as requested. I wasn't able to test OpenMP as you apparently 
can't return from an OpenMP block.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

aaron.ballman wrote:
> haberman wrote:
> > aaron.ballman wrote:
> > > rsmith wrote:
> > > > aaron.ballman wrote:
> > > > > aaron.ballman wrote:
> > > > > > haberman wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > haberman wrote:
> > > > > > > > > haberman wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > haberman wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > This functionality belongs in SemaStmtAttr.cpp, I 
> > > > > > > > > > > > > think.
> > > > > > > > > > > > That is where I had originally put it, but that didn't 
> > > > > > > > > > > > work for templates. The semantic checks can only be 
> > > > > > > > > > > > performed at instantiation time. `ActOnAttributedStmt` 
> > > > > > > > > > > > seems to be the right hook point where I can evaluate 
> > > > > > > > > > > > the semantic checks for both template and non-template 
> > > > > > > > > > > > functions (with template functions getting checked at 
> > > > > > > > > > > > instantiation time).
> > > > > > > > > > > I disagree that `ActOnAttributedStmt()` is the correct 
> > > > > > > > > > > place for this checking -- template checking should occur 
> > > > > > > > > > > when the template is instantiated, same as happens for 
> > > > > > > > > > > declaration attributes. I'd like to see 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 336004.
haberman marked 19 inline comments as done.
haberman added a comment.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

- Returned validation to ActOnAttributedStmt() so it works with templates.
- Merge branch 'main' into musttail
- Address more review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp
  clang/test/Sema/attr-musttail.m

Index: clang/test/Sema/attr-musttail.m
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.m
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -Wno-objc-root-class -verify %s
+
+void TestObjcBlock(void) {
+  void (^x)(void) = ^(void) {
+__attribute__((musttail)) return TestObjcBlock(); // expected-error{{'musttail' attribute cannot be used from a block}}
+  };
+  __attribute__((musttail)) return x();
+}
+
+void ReturnsVoid(void);
+void TestObjcBlockVar(void) {
+  __block int i = 0;  // expected-note{{jump exits scope of __block variable}}
+  __attribute__((musttail)) return ReturnsVoid();  // expected-error{{cannot perform a tail call from this return statement}}
+}
+
+//#import 
+
+__attribute__((objc_root_class))
+@interface TestObjcClass
+@end
+
+@implementation TestObjcClass
+
+- (void)testObjCMethod {
+  __attribute__((musttail)) return ReturnsVoid(); // expected-error{{'musttail' attribute cannot be used from an Objective-C function}}
+}
+
+@end
Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,191 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();   // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;// expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+void NoFunctionCall() {
+  [[clang::musttail]] return;  // expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member of different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is a member of different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+ 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

haberman wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > aaron.ballman wrote:
> > > > aaron.ballman wrote:
> > > > > haberman wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > haberman wrote:
> > > > > > > > haberman wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > haberman wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > This functionality belongs in SemaStmtAttr.cpp, I think.
> > > > > > > > > > > That is where I had originally put it, but that didn't 
> > > > > > > > > > > work for templates. The semantic checks can only be 
> > > > > > > > > > > performed at instantiation time. `ActOnAttributedStmt` 
> > > > > > > > > > > seems to be the right hook point where I can evaluate the 
> > > > > > > > > > > semantic checks for both template and non-template 
> > > > > > > > > > > functions (with template functions getting checked at 
> > > > > > > > > > > instantiation time).
> > > > > > > > > > I disagree that `ActOnAttributedStmt()` is the correct 
> > > > > > > > > > place for this checking -- template checking should occur 
> > > > > > > > > > when the template is instantiated, same as happens for 
> > > > > > > > > > declaration attributes. I'd like to see this functionality 
> > > > > > > > > > moved to SemaStmtAttr.cpp. Keeping the attribute logic 
> > > > > > > > > > together and following the same patterns is what allows us 
> > > > > > > > > > to tablegenerate more of the attribute logic. Statement 
> > > > > > > > > > attributes are just starting to get more such automation.
> > > > > > > > > I tried commenting out this code and adding the following 
> > > > > > > > > code into `handleMustTailAttr()` in `SemaStmtAttr.cpp`:
> > > > > > > > > 
> > > > > > > > > ```
> > > > > > > > >   if (!S.checkMustTailAttr(St, MTA))
> > > > > > > > > return nullptr;
> > > > > > > > > ```
> > > > > > > > > 
> > > > > > > > > This caused my test cases related to templates to fail. It 
> > > > > > > > > also seemed to break test cases related to `JumpDiagnostics`. 
> > > > > > > > > My interpretation of this is that `handleMustTailAttr()` is 
> > > > > > > > > called during parsing only, and cannot catch errors at 
> > > > > > > > > template instantiation time or that require a more complete 
> > > > > > > > > AST.
> > > > > > > > > 
> > > > > > > > > What am I missing? Where in SemaStmtAttr.cpp are you 
> > > > > > > > > suggesting that I put this check?
> > > > > > > > Scratch the part about `JumpDiagnostics`, that was me failing 
> > > > > > > > to call `S.setFunctionHasMustTail()`. I added that and now the 
> > > > > > > > `JumpDiagnostics` tests pass.
> > > > > > > > 
> > > > > > > > But the template test cases still fail, and I can't find any 
> > > > > > > > hook point in `SemaStmtAttr.cpp` that will let me evaluate 
> > > > > > > > these checks at template instantiation time.
> > > > > > > I think there's a bit of an architectural mixup, but I'm curious 
> > > > > > > if @rsmith agrees before anyone starts doing work to make changes.
> > > > > > > 
> > > > > > > When transforming declarations, `RebuildWhatever()` calls the 
> > > > > > > `ActOnWhatever()` function which calls 
> > > > > > > `ProcessDeclAttributeList()` so that attributes are processed. 
> > > > > > > `RebuildAttributedStmt()` similarly calls 
> > > > > > > `ActOnAttributedStmt()`. However, `ActOnAttributedStmt()` doesn't 
> > > > > > > call `ProcessStmtAttributes()` -- the logic is reversed so that 
> > > > > > > `ProcessStmtAttributes()` is what calls `ActOnAttributedStmt()`.
> > > > > > > 
> > > > > > > I think the correct answer is to switch the logic so that 
> > > > > > > `ActOnAttributedStmt()` calls `ProcessStmtAttributes()`, then the 
> > > > > > > template logic should automatically work.
> > > > > > > I think the correct answer is to switch the logic so that 
> > > > > > > ActOnAttributedStmt() calls ProcessStmtAttributes()
> > > > > > 
> > > > > > I think this would require `ProcessStmtAttributes()` to be split 
> > > > > > into two separate functions. Currently that function is doing two 
> > > > > > separate things:
> > > > > > 
> > > > > > 1. Translation of `ParsedAttr` into various subclasses of `Attr`.
> > > > > > 2. Validation that the attribute is semantically valid.
> > > > > > 
> > > > > > The function signature for `ActOnAttributedStmt()` uses `Attr` (not 
> > > > > > `ParsedAttr`), so (1) must happen during the parse, before 
> > > > > > `ActOnAttributedStmt()` is called. But (2) must be deferred until 
> > > > > > template instantiation time for some cases, like `musttail`.
> > > > > I don't think the signature for `ActOnAttributedStmt()` is correct to 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

aaron.ballman wrote:
> rsmith wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > haberman wrote:
> > > > > aaron.ballman wrote:
> > > > > > haberman wrote:
> > > > > > > haberman wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > haberman wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > This functionality belongs in SemaStmtAttr.cpp, I think.
> > > > > > > > > > That is where I had originally put it, but that didn't work 
> > > > > > > > > > for templates. The semantic checks can only be performed at 
> > > > > > > > > > instantiation time. `ActOnAttributedStmt` seems to be the 
> > > > > > > > > > right hook point where I can evaluate the semantic checks 
> > > > > > > > > > for both template and non-template functions (with template 
> > > > > > > > > > functions getting checked at instantiation time).
> > > > > > > > > I disagree that `ActOnAttributedStmt()` is the correct place 
> > > > > > > > > for this checking -- template checking should occur when the 
> > > > > > > > > template is instantiated, same as happens for declaration 
> > > > > > > > > attributes. I'd like to see this functionality moved to 
> > > > > > > > > SemaStmtAttr.cpp. Keeping the attribute logic together and 
> > > > > > > > > following the same patterns is what allows us to 
> > > > > > > > > tablegenerate more of the attribute logic. Statement 
> > > > > > > > > attributes are just starting to get more such automation.
> > > > > > > > I tried commenting out this code and adding the following code 
> > > > > > > > into `handleMustTailAttr()` in `SemaStmtAttr.cpp`:
> > > > > > > > 
> > > > > > > > ```
> > > > > > > >   if (!S.checkMustTailAttr(St, MTA))
> > > > > > > > return nullptr;
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > This caused my test cases related to templates to fail. It also 
> > > > > > > > seemed to break test cases related to `JumpDiagnostics`. My 
> > > > > > > > interpretation of this is that `handleMustTailAttr()` is called 
> > > > > > > > during parsing only, and cannot catch errors at template 
> > > > > > > > instantiation time or that require a more complete AST.
> > > > > > > > 
> > > > > > > > What am I missing? Where in SemaStmtAttr.cpp are you suggesting 
> > > > > > > > that I put this check?
> > > > > > > Scratch the part about `JumpDiagnostics`, that was me failing to 
> > > > > > > call `S.setFunctionHasMustTail()`. I added that and now the 
> > > > > > > `JumpDiagnostics` tests pass.
> > > > > > > 
> > > > > > > But the template test cases still fail, and I can't find any hook 
> > > > > > > point in `SemaStmtAttr.cpp` that will let me evaluate these 
> > > > > > > checks at template instantiation time.
> > > > > > I think there's a bit of an architectural mixup, but I'm curious if 
> > > > > > @rsmith agrees before anyone starts doing work to make changes.
> > > > > > 
> > > > > > When transforming declarations, `RebuildWhatever()` calls the 
> > > > > > `ActOnWhatever()` function which calls `ProcessDeclAttributeList()` 
> > > > > > so that attributes are processed. `RebuildAttributedStmt()` 
> > > > > > similarly calls `ActOnAttributedStmt()`. However, 
> > > > > > `ActOnAttributedStmt()` doesn't call `ProcessStmtAttributes()` -- 
> > > > > > the logic is reversed so that `ProcessStmtAttributes()` is what 
> > > > > > calls `ActOnAttributedStmt()`.
> > > > > > 
> > > > > > I think the correct answer is to switch the logic so that 
> > > > > > `ActOnAttributedStmt()` calls `ProcessStmtAttributes()`, then the 
> > > > > > template logic should automatically work.
> > > > > > I think the correct answer is to switch the logic so that 
> > > > > > ActOnAttributedStmt() calls ProcessStmtAttributes()
> > > > > 
> > > > > I think this would require `ProcessStmtAttributes()` to be split into 
> > > > > two separate functions. Currently that function is doing two separate 
> > > > > things:
> > > > > 
> > > > > 1. Translation of `ParsedAttr` into various subclasses of `Attr`.
> > > > > 2. Validation that the attribute is semantically valid.
> > > > > 
> > > > > The function signature for `ActOnAttributedStmt()` uses `Attr` (not 
> > > > > `ParsedAttr`), so (1) must happen during the parse, before 
> > > > > `ActOnAttributedStmt()` is called. But (2) must be deferred until 
> > > > > template instantiation time for some cases, like `musttail`.
> > > > I don't think the signature for `ActOnAttributedStmt()` is correct to 
> > > > use `Attr` instead of `ParsedAttr`. I think it should be `StmtResult 
> > > > ActOnAttributedStmt(const ParsedAttributesViewWithRange , Stmt 
> > > > *SubStmt);` -- this likely requires a fair bit of surgery 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:588
+
+  const CallExpr *CE = dyn_cast(Ex->IgnoreUnlessSpelledInSource());
+

haberman wrote:
> rsmith wrote:
> > `IgnoreUnlessSpelledInSource` is a syntactic check that's only really 
> > intended for tooling use cases; I think we want something a bit more 
> > semantic here, so `IgnoreImplicitAsWritten` would be more appropriate.
> > 
> > I think it would be reasonable to also skip "parentheses" here (which we 
> > treat as also including things like C's `_Generic`). Would 
> > `Ex->IgnoreImplicitAsWritten()->IgnoreParens()` work?
> > 
> > If we're going to skip elidable copy construction of the result here (which 
> > I think we should), should we also reflect that in the AST? Perhaps we 
> > should strip the return value down to being just the call expression? I'm 
> > thinking in particular of things like building in C++14 or before with 
> > `-fno-elide-constructors`, where code generation for a by-value return of a 
> > class object will synthesize a local temporary to hold the result, with a 
> > final destination copy emitted after the call. (Testcase: `struct A { 
> > A(const A&); }; A f(); A g() { [[clang::musttail]] return f(); }` with 
> > `-fno-elide-constructors`.)
> `IgnoreImplicitAsWritten()` doesn't skip `ExprWithCleanups`, and per your 
> previous comment I was trying to find a `CallExpr` before doing the check 
> prohibiting `ExprWithCleanups` with side effects.
> 
> I could write some custom ignore logic using `clang::IgnoreExprNodes()` 
> directly.
> 
> > If we're going to skip elidable copy construction of the result here (which 
> > I think we should)
> 
> To clarify, are you suggesting that we allow `musttail` through elidable copy 
> constructors on the return value, even if `-fno-elide-constructors` is set? 
> ie. we consider that `musttail` overrides the `-fno-elide-constructors` 
> option on the command line?
> `IgnoreImplicitAsWritten()` doesn't skip `ExprWithCleanups`

That sounds like a bug. Are you sure? It looks like 
`IgnoreImplicitAsWrittenSingleStep` calls `IgnoreImplicitSingleStep` which 
calls `IgnoreImplicitCastsSingleStep` which skips `FullExpr`, and 
`ExprWithCleanups` is a kind of `FullExpr`.

> To clarify, are you suggesting that we allow `musttail` through elidable copy 
> constructors on the return value, even if `-fno-elide-constructors` is set? 
> ie. we consider that `musttail` overrides the `-fno-elide-constructors` 
> option on the command line?

Yes, I think the `musttail` attribute should override 
`-fno-elide-constructors`, because that's necessary in order to provide the 
tail call the user requested (and the local setting should override the global 
one). This is probably worth adding to the documentation.

(Also, `-fno-elide-constructors` is only supposed to affect code generation, 
not language semantics or program validity, so I think either we should always 
reject if a constructor call is required for the return value, regardless of 
whether it's elidable, or we should never reject in that case, and either way 
this determination should be made independent of the setting of 
`-fno-elide-constructors`. Given that choice, it seems more useful to bias 
towards the common case (`-felide-constructors`).)



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

aaron.ballman wrote:
> aaron.ballman wrote:
> > haberman wrote:
> > > aaron.ballman wrote:
> > > > haberman wrote:
> > > > > haberman wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > haberman wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > This functionality belongs in SemaStmtAttr.cpp, I think.
> > > > > > > > That is where I had originally put it, but that didn't work for 
> > > > > > > > templates. The semantic checks can only be performed at 
> > > > > > > > instantiation time. `ActOnAttributedStmt` seems to be the right 
> > > > > > > > hook point where I can evaluate the semantic checks for both 
> > > > > > > > template and non-template functions (with template functions 
> > > > > > > > getting checked at instantiation time).
> > > > > > > I disagree that `ActOnAttributedStmt()` is the correct place for 
> > > > > > > this checking -- template checking should occur when the template 
> > > > > > > is instantiated, same as happens for declaration attributes. I'd 
> > > > > > > like to see this functionality moved to SemaStmtAttr.cpp. Keeping 
> > > > > > > the attribute logic together and following the same patterns is 
> > > > > > > what allows us to tablegenerate more of the attribute logic. 
> > > > > > > Statement attributes are just starting to get more such 
> > > > > > > automation.
> > > > > > I tried commenting out this code and adding 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

aaron.ballman wrote:
> aaron.ballman wrote:
> > haberman wrote:
> > > aaron.ballman wrote:
> > > > haberman wrote:
> > > > > haberman wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > haberman wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > This functionality belongs in SemaStmtAttr.cpp, I think.
> > > > > > > > That is where I had originally put it, but that didn't work for 
> > > > > > > > templates. The semantic checks can only be performed at 
> > > > > > > > instantiation time. `ActOnAttributedStmt` seems to be the right 
> > > > > > > > hook point where I can evaluate the semantic checks for both 
> > > > > > > > template and non-template functions (with template functions 
> > > > > > > > getting checked at instantiation time).
> > > > > > > I disagree that `ActOnAttributedStmt()` is the correct place for 
> > > > > > > this checking -- template checking should occur when the template 
> > > > > > > is instantiated, same as happens for declaration attributes. I'd 
> > > > > > > like to see this functionality moved to SemaStmtAttr.cpp. Keeping 
> > > > > > > the attribute logic together and following the same patterns is 
> > > > > > > what allows us to tablegenerate more of the attribute logic. 
> > > > > > > Statement attributes are just starting to get more such 
> > > > > > > automation.
> > > > > > I tried commenting out this code and adding the following code into 
> > > > > > `handleMustTailAttr()` in `SemaStmtAttr.cpp`:
> > > > > > 
> > > > > > ```
> > > > > >   if (!S.checkMustTailAttr(St, MTA))
> > > > > > return nullptr;
> > > > > > ```
> > > > > > 
> > > > > > This caused my test cases related to templates to fail. It also 
> > > > > > seemed to break test cases related to `JumpDiagnostics`. My 
> > > > > > interpretation of this is that `handleMustTailAttr()` is called 
> > > > > > during parsing only, and cannot catch errors at template 
> > > > > > instantiation time or that require a more complete AST.
> > > > > > 
> > > > > > What am I missing? Where in SemaStmtAttr.cpp are you suggesting 
> > > > > > that I put this check?
> > > > > Scratch the part about `JumpDiagnostics`, that was me failing to call 
> > > > > `S.setFunctionHasMustTail()`. I added that and now the 
> > > > > `JumpDiagnostics` tests pass.
> > > > > 
> > > > > But the template test cases still fail, and I can't find any hook 
> > > > > point in `SemaStmtAttr.cpp` that will let me evaluate these checks at 
> > > > > template instantiation time.
> > > > I think there's a bit of an architectural mixup, but I'm curious if 
> > > > @rsmith agrees before anyone starts doing work to make changes.
> > > > 
> > > > When transforming declarations, `RebuildWhatever()` calls the 
> > > > `ActOnWhatever()` function which calls `ProcessDeclAttributeList()` so 
> > > > that attributes are processed. `RebuildAttributedStmt()` similarly 
> > > > calls `ActOnAttributedStmt()`. However, `ActOnAttributedStmt()` doesn't 
> > > > call `ProcessStmtAttributes()` -- the logic is reversed so that 
> > > > `ProcessStmtAttributes()` is what calls `ActOnAttributedStmt()`.
> > > > 
> > > > I think the correct answer is to switch the logic so that 
> > > > `ActOnAttributedStmt()` calls `ProcessStmtAttributes()`, then the 
> > > > template logic should automatically work.
> > > > I think the correct answer is to switch the logic so that 
> > > > ActOnAttributedStmt() calls ProcessStmtAttributes()
> > > 
> > > I think this would require `ProcessStmtAttributes()` to be split into two 
> > > separate functions. Currently that function is doing two separate things:
> > > 
> > > 1. Translation of `ParsedAttr` into various subclasses of `Attr`.
> > > 2. Validation that the attribute is semantically valid.
> > > 
> > > The function signature for `ActOnAttributedStmt()` uses `Attr` (not 
> > > `ParsedAttr`), so (1) must happen during the parse, before 
> > > `ActOnAttributedStmt()` is called. But (2) must be deferred until 
> > > template instantiation time for some cases, like `musttail`.
> > I don't think the signature for `ActOnAttributedStmt()` is correct to use 
> > `Attr` instead of `ParsedAttr`. I think it should be `StmtResult 
> > ActOnAttributedStmt(const ParsedAttributesViewWithRange , Stmt 
> > *SubStmt);` -- this likely requires a fair bit of surgery to make work 
> > though, which is why I'd like to hear from @rsmith if he agrees with the 
> > approach. In the meantime, I'll play around with this idea locally in more 
> > depth.
> I think my suggestion wasn't quite right, but close. I've got a patch in 
> progress that changes this the way I was thinking it should be changed, but 
> it won't call 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

aaron.ballman wrote:
> haberman wrote:
> > aaron.ballman wrote:
> > > haberman wrote:
> > > > haberman wrote:
> > > > > aaron.ballman wrote:
> > > > > > haberman wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > This functionality belongs in SemaStmtAttr.cpp, I think.
> > > > > > > That is where I had originally put it, but that didn't work for 
> > > > > > > templates. The semantic checks can only be performed at 
> > > > > > > instantiation time. `ActOnAttributedStmt` seems to be the right 
> > > > > > > hook point where I can evaluate the semantic checks for both 
> > > > > > > template and non-template functions (with template functions 
> > > > > > > getting checked at instantiation time).
> > > > > > I disagree that `ActOnAttributedStmt()` is the correct place for 
> > > > > > this checking -- template checking should occur when the template 
> > > > > > is instantiated, same as happens for declaration attributes. I'd 
> > > > > > like to see this functionality moved to SemaStmtAttr.cpp. Keeping 
> > > > > > the attribute logic together and following the same patterns is 
> > > > > > what allows us to tablegenerate more of the attribute logic. 
> > > > > > Statement attributes are just starting to get more such automation.
> > > > > I tried commenting out this code and adding the following code into 
> > > > > `handleMustTailAttr()` in `SemaStmtAttr.cpp`:
> > > > > 
> > > > > ```
> > > > >   if (!S.checkMustTailAttr(St, MTA))
> > > > > return nullptr;
> > > > > ```
> > > > > 
> > > > > This caused my test cases related to templates to fail. It also 
> > > > > seemed to break test cases related to `JumpDiagnostics`. My 
> > > > > interpretation of this is that `handleMustTailAttr()` is called 
> > > > > during parsing only, and cannot catch errors at template 
> > > > > instantiation time or that require a more complete AST.
> > > > > 
> > > > > What am I missing? Where in SemaStmtAttr.cpp are you suggesting that 
> > > > > I put this check?
> > > > Scratch the part about `JumpDiagnostics`, that was me failing to call 
> > > > `S.setFunctionHasMustTail()`. I added that and now the 
> > > > `JumpDiagnostics` tests pass.
> > > > 
> > > > But the template test cases still fail, and I can't find any hook point 
> > > > in `SemaStmtAttr.cpp` that will let me evaluate these checks at 
> > > > template instantiation time.
> > > I think there's a bit of an architectural mixup, but I'm curious if 
> > > @rsmith agrees before anyone starts doing work to make changes.
> > > 
> > > When transforming declarations, `RebuildWhatever()` calls the 
> > > `ActOnWhatever()` function which calls `ProcessDeclAttributeList()` so 
> > > that attributes are processed. `RebuildAttributedStmt()` similarly calls 
> > > `ActOnAttributedStmt()`. However, `ActOnAttributedStmt()` doesn't call 
> > > `ProcessStmtAttributes()` -- the logic is reversed so that 
> > > `ProcessStmtAttributes()` is what calls `ActOnAttributedStmt()`.
> > > 
> > > I think the correct answer is to switch the logic so that 
> > > `ActOnAttributedStmt()` calls `ProcessStmtAttributes()`, then the 
> > > template logic should automatically work.
> > > I think the correct answer is to switch the logic so that 
> > > ActOnAttributedStmt() calls ProcessStmtAttributes()
> > 
> > I think this would require `ProcessStmtAttributes()` to be split into two 
> > separate functions. Currently that function is doing two separate things:
> > 
> > 1. Translation of `ParsedAttr` into various subclasses of `Attr`.
> > 2. Validation that the attribute is semantically valid.
> > 
> > The function signature for `ActOnAttributedStmt()` uses `Attr` (not 
> > `ParsedAttr`), so (1) must happen during the parse, before 
> > `ActOnAttributedStmt()` is called. But (2) must be deferred until template 
> > instantiation time for some cases, like `musttail`.
> I don't think the signature for `ActOnAttributedStmt()` is correct to use 
> `Attr` instead of `ParsedAttr`. I think it should be `StmtResult 
> ActOnAttributedStmt(const ParsedAttributesViewWithRange , Stmt 
> *SubStmt);` -- this likely requires a fair bit of surgery to make work 
> though, which is why I'd like to hear from @rsmith if he agrees with the 
> approach. In the meantime, I'll play around with this idea locally in more 
> depth.
I think my suggestion wasn't quite right, but close. I've got a patch in 
progress that changes this the way I was thinking it should be changed, but it 
won't call `ActOnAttributedStmt()` when doing template instantiation. Instead, 
it will continue to instantiate attributes explicitly by calling 
`TransformAttr()` and any additional instantiation time checks will 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

haberman wrote:
> aaron.ballman wrote:
> > haberman wrote:
> > > haberman wrote:
> > > > aaron.ballman wrote:
> > > > > haberman wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > This functionality belongs in SemaStmtAttr.cpp, I think.
> > > > > > That is where I had originally put it, but that didn't work for 
> > > > > > templates. The semantic checks can only be performed at 
> > > > > > instantiation time. `ActOnAttributedStmt` seems to be the right 
> > > > > > hook point where I can evaluate the semantic checks for both 
> > > > > > template and non-template functions (with template functions 
> > > > > > getting checked at instantiation time).
> > > > > I disagree that `ActOnAttributedStmt()` is the correct place for this 
> > > > > checking -- template checking should occur when the template is 
> > > > > instantiated, same as happens for declaration attributes. I'd like to 
> > > > > see this functionality moved to SemaStmtAttr.cpp. Keeping the 
> > > > > attribute logic together and following the same patterns is what 
> > > > > allows us to tablegenerate more of the attribute logic. Statement 
> > > > > attributes are just starting to get more such automation.
> > > > I tried commenting out this code and adding the following code into 
> > > > `handleMustTailAttr()` in `SemaStmtAttr.cpp`:
> > > > 
> > > > ```
> > > >   if (!S.checkMustTailAttr(St, MTA))
> > > > return nullptr;
> > > > ```
> > > > 
> > > > This caused my test cases related to templates to fail. It also seemed 
> > > > to break test cases related to `JumpDiagnostics`. My interpretation of 
> > > > this is that `handleMustTailAttr()` is called during parsing only, and 
> > > > cannot catch errors at template instantiation time or that require a 
> > > > more complete AST.
> > > > 
> > > > What am I missing? Where in SemaStmtAttr.cpp are you suggesting that I 
> > > > put this check?
> > > Scratch the part about `JumpDiagnostics`, that was me failing to call 
> > > `S.setFunctionHasMustTail()`. I added that and now the `JumpDiagnostics` 
> > > tests pass.
> > > 
> > > But the template test cases still fail, and I can't find any hook point 
> > > in `SemaStmtAttr.cpp` that will let me evaluate these checks at template 
> > > instantiation time.
> > I think there's a bit of an architectural mixup, but I'm curious if @rsmith 
> > agrees before anyone starts doing work to make changes.
> > 
> > When transforming declarations, `RebuildWhatever()` calls the 
> > `ActOnWhatever()` function which calls `ProcessDeclAttributeList()` so that 
> > attributes are processed. `RebuildAttributedStmt()` similarly calls 
> > `ActOnAttributedStmt()`. However, `ActOnAttributedStmt()` doesn't call 
> > `ProcessStmtAttributes()` -- the logic is reversed so that 
> > `ProcessStmtAttributes()` is what calls `ActOnAttributedStmt()`.
> > 
> > I think the correct answer is to switch the logic so that 
> > `ActOnAttributedStmt()` calls `ProcessStmtAttributes()`, then the template 
> > logic should automatically work.
> > I think the correct answer is to switch the logic so that 
> > ActOnAttributedStmt() calls ProcessStmtAttributes()
> 
> I think this would require `ProcessStmtAttributes()` to be split into two 
> separate functions. Currently that function is doing two separate things:
> 
> 1. Translation of `ParsedAttr` into various subclasses of `Attr`.
> 2. Validation that the attribute is semantically valid.
> 
> The function signature for `ActOnAttributedStmt()` uses `Attr` (not 
> `ParsedAttr`), so (1) must happen during the parse, before 
> `ActOnAttributedStmt()` is called. But (2) must be deferred until template 
> instantiation time for some cases, like `musttail`.
I don't think the signature for `ActOnAttributedStmt()` is correct to use 
`Attr` instead of `ParsedAttr`. I think it should be `StmtResult 
ActOnAttributedStmt(const ParsedAttributesViewWithRange , Stmt 
*SubStmt);` -- this likely requires a fair bit of surgery to make work though, 
which is why I'd like to hear from @rsmith if he agrees with the approach. In 
the meantime, I'll play around with this idea locally in more depth.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

aaron.ballman wrote:
> haberman wrote:
> > haberman wrote:
> > > aaron.ballman wrote:
> > > > haberman wrote:
> > > > > aaron.ballman wrote:
> > > > > > This functionality belongs in SemaStmtAttr.cpp, I think.
> > > > > That is where I had originally put it, but that didn't work for 
> > > > > templates. The semantic checks can only be performed at instantiation 
> > > > > time. `ActOnAttributedStmt` seems to be the right hook point where I 
> > > > > can evaluate the semantic checks for both template and non-template 
> > > > > functions (with template functions getting checked at instantiation 
> > > > > time).
> > > > I disagree that `ActOnAttributedStmt()` is the correct place for this 
> > > > checking -- template checking should occur when the template is 
> > > > instantiated, same as happens for declaration attributes. I'd like to 
> > > > see this functionality moved to SemaStmtAttr.cpp. Keeping the attribute 
> > > > logic together and following the same patterns is what allows us to 
> > > > tablegenerate more of the attribute logic. Statement attributes are 
> > > > just starting to get more such automation.
> > > I tried commenting out this code and adding the following code into 
> > > `handleMustTailAttr()` in `SemaStmtAttr.cpp`:
> > > 
> > > ```
> > >   if (!S.checkMustTailAttr(St, MTA))
> > > return nullptr;
> > > ```
> > > 
> > > This caused my test cases related to templates to fail. It also seemed to 
> > > break test cases related to `JumpDiagnostics`. My interpretation of this 
> > > is that `handleMustTailAttr()` is called during parsing only, and cannot 
> > > catch errors at template instantiation time or that require a more 
> > > complete AST.
> > > 
> > > What am I missing? Where in SemaStmtAttr.cpp are you suggesting that I 
> > > put this check?
> > Scratch the part about `JumpDiagnostics`, that was me failing to call 
> > `S.setFunctionHasMustTail()`. I added that and now the `JumpDiagnostics` 
> > tests pass.
> > 
> > But the template test cases still fail, and I can't find any hook point in 
> > `SemaStmtAttr.cpp` that will let me evaluate these checks at template 
> > instantiation time.
> I think there's a bit of an architectural mixup, but I'm curious if @rsmith 
> agrees before anyone starts doing work to make changes.
> 
> When transforming declarations, `RebuildWhatever()` calls the 
> `ActOnWhatever()` function which calls `ProcessDeclAttributeList()` so that 
> attributes are processed. `RebuildAttributedStmt()` similarly calls 
> `ActOnAttributedStmt()`. However, `ActOnAttributedStmt()` doesn't call 
> `ProcessStmtAttributes()` -- the logic is reversed so that 
> `ProcessStmtAttributes()` is what calls `ActOnAttributedStmt()`.
> 
> I think the correct answer is to switch the logic so that 
> `ActOnAttributedStmt()` calls `ProcessStmtAttributes()`, then the template 
> logic should automatically work.
> I think the correct answer is to switch the logic so that 
> ActOnAttributedStmt() calls ProcessStmtAttributes()

I think this would require `ProcessStmtAttributes()` to be split into two 
separate functions. Currently that function is doing two separate things:

1. Translation of `ParsedAttr` into various subclasses of `Attr`.
2. Validation that the attribute is semantically valid.

The function signature for `ActOnAttributedStmt()` uses `Attr` (not 
`ParsedAttr`), so (1) must happen during the parse, before 
`ActOnAttributedStmt()` is called. But (2) must be deferred until template 
instantiation time for some cases, like `musttail`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

haberman wrote:
> haberman wrote:
> > aaron.ballman wrote:
> > > haberman wrote:
> > > > aaron.ballman wrote:
> > > > > This functionality belongs in SemaStmtAttr.cpp, I think.
> > > > That is where I had originally put it, but that didn't work for 
> > > > templates. The semantic checks can only be performed at instantiation 
> > > > time. `ActOnAttributedStmt` seems to be the right hook point where I 
> > > > can evaluate the semantic checks for both template and non-template 
> > > > functions (with template functions getting checked at instantiation 
> > > > time).
> > > I disagree that `ActOnAttributedStmt()` is the correct place for this 
> > > checking -- template checking should occur when the template is 
> > > instantiated, same as happens for declaration attributes. I'd like to see 
> > > this functionality moved to SemaStmtAttr.cpp. Keeping the attribute logic 
> > > together and following the same patterns is what allows us to 
> > > tablegenerate more of the attribute logic. Statement attributes are just 
> > > starting to get more such automation.
> > I tried commenting out this code and adding the following code into 
> > `handleMustTailAttr()` in `SemaStmtAttr.cpp`:
> > 
> > ```
> >   if (!S.checkMustTailAttr(St, MTA))
> > return nullptr;
> > ```
> > 
> > This caused my test cases related to templates to fail. It also seemed to 
> > break test cases related to `JumpDiagnostics`. My interpretation of this is 
> > that `handleMustTailAttr()` is called during parsing only, and cannot catch 
> > errors at template instantiation time or that require a more complete AST.
> > 
> > What am I missing? Where in SemaStmtAttr.cpp are you suggesting that I put 
> > this check?
> Scratch the part about `JumpDiagnostics`, that was me failing to call 
> `S.setFunctionHasMustTail()`. I added that and now the `JumpDiagnostics` 
> tests pass.
> 
> But the template test cases still fail, and I can't find any hook point in 
> `SemaStmtAttr.cpp` that will let me evaluate these checks at template 
> instantiation time.
I think there's a bit of an architectural mixup, but I'm curious if @rsmith 
agrees before anyone starts doing work to make changes.

When transforming declarations, `RebuildWhatever()` calls the `ActOnWhatever()` 
function which calls `ProcessDeclAttributeList()` so that attributes are 
processed. `RebuildAttributedStmt()` similarly calls `ActOnAttributedStmt()`. 
However, `ActOnAttributedStmt()` doesn't call `ProcessStmtAttributes()` -- the 
logic is reversed so that `ProcessStmtAttributes()` is what calls 
`ActOnAttributedStmt()`.

I think the correct answer is to switch the logic so that 
`ActOnAttributedStmt()` calls `ProcessStmtAttributes()`, then the template 
logic should automatically work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

haberman wrote:
> aaron.ballman wrote:
> > haberman wrote:
> > > aaron.ballman wrote:
> > > > This functionality belongs in SemaStmtAttr.cpp, I think.
> > > That is where I had originally put it, but that didn't work for 
> > > templates. The semantic checks can only be performed at instantiation 
> > > time. `ActOnAttributedStmt` seems to be the right hook point where I can 
> > > evaluate the semantic checks for both template and non-template functions 
> > > (with template functions getting checked at instantiation time).
> > I disagree that `ActOnAttributedStmt()` is the correct place for this 
> > checking -- template checking should occur when the template is 
> > instantiated, same as happens for declaration attributes. I'd like to see 
> > this functionality moved to SemaStmtAttr.cpp. Keeping the attribute logic 
> > together and following the same patterns is what allows us to tablegenerate 
> > more of the attribute logic. Statement attributes are just starting to get 
> > more such automation.
> I tried commenting out this code and adding the following code into 
> `handleMustTailAttr()` in `SemaStmtAttr.cpp`:
> 
> ```
>   if (!S.checkMustTailAttr(St, MTA))
> return nullptr;
> ```
> 
> This caused my test cases related to templates to fail. It also seemed to 
> break test cases related to `JumpDiagnostics`. My interpretation of this is 
> that `handleMustTailAttr()` is called during parsing only, and cannot catch 
> errors at template instantiation time or that require a more complete AST.
> 
> What am I missing? Where in SemaStmtAttr.cpp are you suggesting that I put 
> this check?
Scratch the part about `JumpDiagnostics`, that was me failing to call 
`S.setFunctionHasMustTail()`. I added that and now the `JumpDiagnostics` tests 
pass.

But the template test cases still fail, and I can't find any hook point in 
`SemaStmtAttr.cpp` that will let me evaluate these checks at template 
instantiation time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 335106.
haberman added a comment.

- Added missing S.setFunctionHasMustTail().


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp
  clang/test/Sema/attr-musttail.m

Index: clang/test/Sema/attr-musttail.m
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.m
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -verify %s
+
+void TestObjcBlock(void) {
+  void (^x)(void) = ^(void) {
+__attribute__((musttail)) return TestObjcBlock(); // expected-error{{'musttail' attribute cannot be used from Objective-C blocks}}
+  };
+  __attribute__((musttail)) return x();
+}
Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,192 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();   // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;// expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member of different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is a member of different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsVoid(); // expected-error {{cannot perform a tail call from this return statement}}
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+int TestNonTrivialParam(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{'musttail' attribute requires that the return value, all parameters, and any temporaries created by the expression are trivially destructible and do not require ARC}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+HasNonTrivialDestructor TestReturnsNonTrivialValue() {
+  [[clang::musttail]] return (ReturnsNonTrivialValue()); // expected-error {{'musttail' attribute requires that the return value, all 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:588
+
+  const CallExpr *CE = dyn_cast(Ex->IgnoreUnlessSpelledInSource());
+

rsmith wrote:
> `IgnoreUnlessSpelledInSource` is a syntactic check that's only really 
> intended for tooling use cases; I think we want something a bit more semantic 
> here, so `IgnoreImplicitAsWritten` would be more appropriate.
> 
> I think it would be reasonable to also skip "parentheses" here (which we 
> treat as also including things like C's `_Generic`). Would 
> `Ex->IgnoreImplicitAsWritten()->IgnoreParens()` work?
> 
> If we're going to skip elidable copy construction of the result here (which I 
> think we should), should we also reflect that in the AST? Perhaps we should 
> strip the return value down to being just the call expression? I'm thinking 
> in particular of things like building in C++14 or before with 
> `-fno-elide-constructors`, where code generation for a by-value return of a 
> class object will synthesize a local temporary to hold the result, with a 
> final destination copy emitted after the call. (Testcase: `struct A { A(const 
> A&); }; A f(); A g() { [[clang::musttail]] return f(); }` with 
> `-fno-elide-constructors`.)
`IgnoreImplicitAsWritten()` doesn't skip `ExprWithCleanups`, and per your 
previous comment I was trying to find a `CallExpr` before doing the check 
prohibiting `ExprWithCleanups` with side effects.

I could write some custom ignore logic using `clang::IgnoreExprNodes()` 
directly.

> If we're going to skip elidable copy construction of the result here (which I 
> think we should)

To clarify, are you suggesting that we allow `musttail` through elidable copy 
constructors on the return value, even if `-fno-elide-constructors` is set? ie. 
we consider that `musttail` overrides the `-fno-elide-constructors` option on 
the command line?



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

aaron.ballman wrote:
> haberman wrote:
> > aaron.ballman wrote:
> > > This functionality belongs in SemaStmtAttr.cpp, I think.
> > That is where I had originally put it, but that didn't work for templates. 
> > The semantic checks can only be performed at instantiation time. 
> > `ActOnAttributedStmt` seems to be the right hook point where I can evaluate 
> > the semantic checks for both template and non-template functions (with 
> > template functions getting checked at instantiation time).
> I disagree that `ActOnAttributedStmt()` is the correct place for this 
> checking -- template checking should occur when the template is instantiated, 
> same as happens for declaration attributes. I'd like to see this 
> functionality moved to SemaStmtAttr.cpp. Keeping the attribute logic together 
> and following the same patterns is what allows us to tablegenerate more of 
> the attribute logic. Statement attributes are just starting to get more such 
> automation.
I tried commenting out this code and adding the following code into 
`handleMustTailAttr()` in `SemaStmtAttr.cpp`:

```
  if (!S.checkMustTailAttr(St, MTA))
return nullptr;
```

This caused my test cases related to templates to fail. It also seemed to break 
test cases related to `JumpDiagnostics`. My interpretation of this is that 
`handleMustTailAttr()` is called during parsing only, and cannot catch errors 
at template instantiation time or that require a more complete AST.

What am I missing? Where in SemaStmtAttr.cpp are you suggesting that I put this 
check?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 335103.
haberman marked 3 inline comments as done.
haberman added a comment.

- Addressed comments and tried moving check to SemaStmtAttr.cpp.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp
  clang/test/Sema/attr-musttail.m

Index: clang/test/Sema/attr-musttail.m
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.m
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -verify %s
+
+void TestObjcBlock(void) {
+  void (^x)(void) = ^(void) {
+__attribute__((musttail)) return TestObjcBlock(); // expected-error{{'musttail' attribute cannot be used from Objective-C blocks}}
+  };
+  __attribute__((musttail)) return x();
+}
Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,192 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();   // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;// expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member of different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is a member of different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsVoid(); // expected-error {{cannot perform a tail call from this return statement}}
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+int TestNonTrivialParam(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{'musttail' attribute requires that the return value, all parameters, and any temporaries created by the expression are trivially destructible and do not require ARC}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+HasNonTrivialDestructor TestReturnsNonTrivialValue() {
+  [[clang::musttail]] return (ReturnsNonTrivialValue()); // expected-error 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:458
+same number of arguments as the caller. The types of the return value and all
+arguments must be similar, including the implicit "this" argument, if any.
+Any variables in scope, including all arguments to the function and the

It'd be nice if we could nail down "similar" somewhat. I don't know if `int` 
and `short` are similar (due to promotions) or `const int` and `int` are 
similar, etc.



Comment at: clang/include/clang/Basic/AttrDocs.td:461-462
+return value must be trivially destructible. The calling convention of the
+caller and callee must match, and they must not have old style K C function
+declarations.
+  }];

Not only is this not usable with K C declarations, but it's also not usable 
with `...` variadic functions either, right?



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

haberman wrote:
> aaron.ballman wrote:
> > This functionality belongs in SemaStmtAttr.cpp, I think.
> That is where I had originally put it, but that didn't work for templates. 
> The semantic checks can only be performed at instantiation time. 
> `ActOnAttributedStmt` seems to be the right hook point where I can evaluate 
> the semantic checks for both template and non-template functions (with 
> template functions getting checked at instantiation time).
I disagree that `ActOnAttributedStmt()` is the correct place for this checking 
-- template checking should occur when the template is instantiated, same as 
happens for declaration attributes. I'd like to see this functionality moved to 
SemaStmtAttr.cpp. Keeping the attribute logic together and following the same 
patterns is what allows us to tablegenerate more of the attribute logic. 
Statement attributes are just starting to get more such automation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D99517#2667088 , @rsmith wrote:

> In D99517#2667025 , @rjmccall wrote:
>
>> You should structure this code so it's easy to add exceptions for certain 
>> calling conventions that can support tail calls with weaker restrictions 
>> (principally, callee-pop conventions).  Mostly that probably means checking 
>> the calling convention first, or extracting the type restriction checks into 
>> a different function that you can skip.  For example, I believe x86's 
>> `fastcall` convention can logically support any combination of prototypes as 
>> `musttail` as long as the return types are vaguely compatible.
>
> The LLVM `musttail` flag doesn't seem to allow for any target-specific 
> loosening of the rules at the moment, so I don't think we can get any benefit 
> from such restructuring right now; do you think it's OK to defer this 
> restructuring and use the stricter rules across all targets for now?

Right, I wasn't suggesting that we needed to implement weaker rules right now, 
just that it'd be nice if the code didn't have to be totally restructured just 
to do it.  Right now it's one big function that does all the checks.

> I think there is also value in having a target-independent set of 
> restrictions, even if we could actually guarantee tail calls in more 
> circumstances on some (or maybe most!) targets, in order to allow people to 
> make portable use of the attribute and as data towards something that we 
> might be able to standardize. (For example, the people working on coroutines 
> in C++ wanted something like this, but wanted feedback from implementers on 
> what set of restrictions would be necessary in order to portably guarantee a 
> tail call.) In order to strike a balance between portability and usefulness 
> here, maybe we could plan to eventually accept any musttail call we know the 
> target can support, but warn on musttail calls that don't satisfy the 
> stricter rules and therefore may be non-portable?

I agree that we should not start loosening restrictions based on the vagaries 
of the platform CC, e.g. recognizing that a particular set of arguments happens 
to be passed solely in registers.  I was thinking about callee-pop CCs, like 
`fastcall` and `swiftasynccall`, which are generally designed from the start to 
support almost unrestricted tail calls; e.g. the only restriction on tail calls 
between `fastcall` functions is that the return types are compatible.  (IIRC — 
it's possible that highly-aligned arguments would change that.)  Since tail 
calls are part of the designed feature set of these conventions, it seems 
appropriate to think about them when adding a tail-call feature.

Standard C conventions generally don't support unrestricted tail calls (because 
of variadics, unprototyped calls, easier assembly-writing, and history), so 
this would only apply as a target-specific extension when used in conjunction 
with a non-standard CC, which meshes well with your goals for standardization.  
I just want you to write the code so that maintainers can more easily skip some 
of the restrictions to cover a non-standard CC.

I'm not surprised that the C++ coroutine people want unrestricted tail calls; 
this is all pretty predictable, and it's essentially the point I made about 
generic coroutine lowering several years ago at LLVM dev.  Really, they need to 
be asking for a standard calling convention that guarantees unrestricted tail 
calls.  Of course, that would require the standard to admit the existence of 
calling conventions (other than language linkage :)).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D99517#2667025 , @rjmccall wrote:

> You should structure this code so it's easy to add exceptions for certain 
> calling conventions that can support tail calls with weaker restrictions 
> (principally, callee-pop conventions).  Mostly that probably means checking 
> the calling convention first, or extracting the type restriction checks into 
> a different function that you can skip.  For example, I believe x86's 
> `fastcall` convention can logically support any combination of prototypes as 
> `musttail` as long as the return types are vaguely compatible.

The LLVM `musttail` flag doesn't seem to allow for any target-specific 
loosening of the rules at the moment, so I don't think we can get any benefit 
from such restructuring right now; do you think it's OK to defer this 
restructuring and use the stricter rules across all targets for now?

I think there is also value in having a target-independent set of restrictions, 
even if we could actually guarantee tail calls in more circumstances on some 
(or maybe most!) targets, in order to allow people to make portable use of the 
attribute and as data towards something that we might be able to standardize. 
(For example, the people working on coroutines in C++ wanted something like 
this, but wanted feedback from implementers on what set of restrictions would 
be necessary in order to portably guarantee a tail call.) In order to strike a 
balance between portability and usefulness here, maybe we could plan to 
eventually accept any musttail call we know the target can support, but warn on 
musttail calls that don't satisfy the stricter rules and therefore may be 
non-portable?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[John McCall via Phabricator via cfe-commits]

rjmccall added a reviewer: varungandhi-apple.
rjmccall added a comment.

CC'ing Varun Gandhi.

Is `musttail` actually supported generically on all LLVM backends, or does this 
need a target restriction?

You should structure this code so it's easy to add exceptions for certain 
calling conventions that can support tail calls with weaker restrictions 
(principally, callee-pop conventions).  Mostly that probably means checking the 
calling convention first, or extracting the type restriction checks into a 
different function that you can skip.  For example, I believe x86's `fastcall` 
convention can logically support any combination of prototypes as `musttail` as 
long as the return types are vaguely compatible.




Comment at: clang/lib/CodeGen/CGCall.cpp:5318-5319
+  EHCleanupScope *Cleanup = dyn_cast(&*it);
+  assert(Cleanup && Cleanup->getCleanup()->isCallLifetimeEnd() &&
+ "found unexpected cleanup generating musttail exit");
+}

rsmith wrote:
> Given the potential for mismatch between the JumpDiagnostics checks and this 
> one, especially as new more exotic kinds of cleanup are added, I wonder if we 
> should use an `ErrorUnsupported` here instead of an `assert`.
> 
> I strongly suspect we can still reach the problematic case here for a tail 
> call in a statement expression. I don't think it's feasible to check for all 
> the ways that an arbitrary expression context can have pending cleanups, 
> which we'd need in order to produce precise `Sema` diagnostics for that, so 
> either we handle that here or we blanket reject all `musttail` returns in 
> statement expressions. I think either approach is probably acceptable.
Yes, I think ErrorUnsupported is a much better idea.



Comment at: clang/lib/Sema/SemaStmt.cpp:631
+// Caller is an Obj-C block decl: ^(void) { /* ... */ }
+assert(dyn_cast(CurContext) && "unexpected decl context");
+Diag(St->getBeginLoc(), diag::err_musttail_from_block_forbidden) << 

rsmith wrote:
> There are a couple of other contexts that can include a return statement: the 
> caller could also be an `ObjCMethodDecl` (an Objective-C method) or a 
> `CapturedDecl` (the body of a `#pragma omp` parallel region). I'd probably 
> use a specific diagnostic ("cannot be used from a block" / "cannot be used 
> from an Objective-C function") for the block and ObjCMethod case, and a 
> nonsepcific-but-correct "cannot be used from this context" for anything else.
Blocks ought to be extremely straightforward to support.  Just validate that 
the tail call is to a block pointer and then compare the underlying function 
types line up in the same way.  You will need to be able to verify that there 
isn't a non-trivial conversion on the return types, even if the return type 
isn't known at this point in the function, but that's a problem in C++ as well 
due to lambdas and `auto` deduced return types.

Also, you can use `isa<...>` for checks like this instead of `dyn_cast<...>`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:5315-5317
+// TODO(haberman): insert checks/assertions to verify that this early exit
+// is safe. We tried to verify this in Sema but we should double-check
+// here.

rsmith wrote:
> haberman wrote:
> > rsmith wrote:
> > > haberman wrote:
> > > > aaron.ballman wrote:
> > > > > Are you planning to handle this TODO in the patch? If not, can you 
> > > > > switch to a FIXME without a name associated with it?
> > > > I am interested in feedback on the best way to proceed here.
> > > > 
> > > >   - Is my assessment correct that we should have an assertion that 
> > > > validates this?
> > > >   - Is such an assertion reasonably feasible to implement?
> > > >   - Is it ok to defer with FIXME, or should I try to fix it in this 
> > > > patch?
> > > > 
> > > > I've changed it to a FIXME for now.
> > > Yes, I think we should validate this by an assertion if we can. We can 
> > > check this by walking the cleanup scope stack (walk from 
> > > `CurrentCleanupScopeDepth` to `EHScopeStack::stable_end()`) and making 
> > > sure that there is no "problematic" enclosing cleanup scope. Here, 
> > > "problematic" would mean any scope other than an `EHCleanupScope` 
> > > containing only `CallLifetimeEnd` cleanups.
> > > 
> > > Looking at the kinds of cleanups that we might encounter here, I think 
> > > there may be a few more things that Sema needs to check in order to not 
> > > get in the way of exception handling. In particular, I think we should 
> > > reject if the callee is potentially-throwing and the musttail call is 
> > > inside a try block or a function that's either noexcept or has a dynamic 
> > > exception specification.
> > > 
> > > Oh, also, we should disallow musttail calls inside statement expressions, 
> > > in order to defend against cleanups that exist transiently within an 
> > > expression.
> > I'm having trouble implementing the check because there doesn't appear to 
> > be any discriminator in `EHScopeStack::Cleanup` that will let you test if 
> > it is a `CallLifetimeEnd`. (The actual code just does virtual dispatch 
> > through `EHScopeStack::Cleanup::Emit()`.
> > 
> > I temporarily implemented this by adding an extra virtual function to act 
> > as discriminator. The check fires if a VLA is in scope:
> > 
> > ```
> > int Func14(int x) {
> >   int vla[x];
> >   [[clang::musttail]] return Bar(x);
> > }
> > ```
> > 
> > Do we need to forbid VLAs or do I need to refine the check?
> > 
> > It appears that `JumpDiagnostics.cpp` is already diagnosing statement 
> > expressions and `try`. However I could not get testing to work. I tried 
> > adding a test with `try`  but even with `-fexceptions` I am getting:
> > 
> > ```
> > cannot use 'try' with exceptions disabled
> > ```
> > Do we need to forbid VLAs or do I need to refine the check?
> 
> Assuming that LLVM supports musttail calls from functions where a dynamic 
> alloca is in scope, I think we should allow VLAs. The `musttail` 
> documentation doesn't mention this, so I think its OK, and I can't think of a 
> good reason why you wouldn't be able to `musttail` call due to a 
> variably-sized frame.
> 
> Perhaps a good model would be to add a virtual function to permit asking a 
> cleanup whether it's optional / skippable.
> 
> > I could not get testing to work.
> 
> You need `-fcxx-exceptions` to use `try`. At the `-cc1` level, we have 
> essentially-orthogonal settings for "it's valid for exceptions to unwind 
> through this code" (`-fexceptions`) and "C++ exception handling syntax is 
> permitted" (`-fcxx-exceptions`), and you usually need to enable both for 
> CodeGen tests involving exceptions.
Or maybe instead of "is optional / skippable", the right question is, "is this 
redundant if we're about to return?" That way we could potentially one day 
reuse the same mechanism to also skip emitting such cleanups when emitting a 
cleanup path into the return block.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, I think this is looking really good.

@rjmccall, no explicit need to review; I just wanted to make sure you'd seen 
this and had a chance to express any concerns before we go ahead.




Comment at: clang/include/clang/Basic/AttrDocs.td:452
+optimizations are disabled. This guarantees that the call will not cause
+unbounded stack growth if it is part of a recursive cycle in the call graph.
+

One thing I'd add:

> If the callee is a virtual function that is implemented by a thunk, there is 
> no guarantee in general that the thunk tail-calls the implementation of the 
> virtual function, so such a call in a recursive cycle can still result in 
> unbounded stack growth.



Comment at: clang/lib/CodeGen/CGCall.cpp:5318-5319
+  EHCleanupScope *Cleanup = dyn_cast(&*it);
+  assert(Cleanup && Cleanup->getCleanup()->isCallLifetimeEnd() &&
+ "found unexpected cleanup generating musttail exit");
+}

Given the potential for mismatch between the JumpDiagnostics checks and this 
one, especially as new more exotic kinds of cleanup are added, I wonder if we 
should use an `ErrorUnsupported` here instead of an `assert`.

I strongly suspect we can still reach the problematic case here for a tail call 
in a statement expression. I don't think it's feasible to check for all the 
ways that an arbitrary expression context can have pending cleanups, which we'd 
need in order to produce precise `Sema` diagnostics for that, so either we 
handle that here or we blanket reject all `musttail` returns in statement 
expressions. I think either approach is probably acceptable.



Comment at: clang/lib/CodeGen/CGCall.cpp:5315-5317
+// TODO(haberman): insert checks/assertions to verify that this early exit
+// is safe. We tried to verify this in Sema but we should double-check
+// here.

haberman wrote:
> rsmith wrote:
> > haberman wrote:
> > > aaron.ballman wrote:
> > > > Are you planning to handle this TODO in the patch? If not, can you 
> > > > switch to a FIXME without a name associated with it?
> > > I am interested in feedback on the best way to proceed here.
> > > 
> > >   - Is my assessment correct that we should have an assertion that 
> > > validates this?
> > >   - Is such an assertion reasonably feasible to implement?
> > >   - Is it ok to defer with FIXME, or should I try to fix it in this patch?
> > > 
> > > I've changed it to a FIXME for now.
> > Yes, I think we should validate this by an assertion if we can. We can 
> > check this by walking the cleanup scope stack (walk from 
> > `CurrentCleanupScopeDepth` to `EHScopeStack::stable_end()`) and making sure 
> > that there is no "problematic" enclosing cleanup scope. Here, "problematic" 
> > would mean any scope other than an `EHCleanupScope` containing only 
> > `CallLifetimeEnd` cleanups.
> > 
> > Looking at the kinds of cleanups that we might encounter here, I think 
> > there may be a few more things that Sema needs to check in order to not get 
> > in the way of exception handling. In particular, I think we should reject 
> > if the callee is potentially-throwing and the musttail call is inside a try 
> > block or a function that's either noexcept or has a dynamic exception 
> > specification.
> > 
> > Oh, also, we should disallow musttail calls inside statement expressions, 
> > in order to defend against cleanups that exist transiently within an 
> > expression.
> I'm having trouble implementing the check because there doesn't appear to be 
> any discriminator in `EHScopeStack::Cleanup` that will let you test if it is 
> a `CallLifetimeEnd`. (The actual code just does virtual dispatch through 
> `EHScopeStack::Cleanup::Emit()`.
> 
> I temporarily implemented this by adding an extra virtual function to act as 
> discriminator. The check fires if a VLA is in scope:
> 
> ```
> int Func14(int x) {
>   int vla[x];
>   [[clang::musttail]] return Bar(x);
> }
> ```
> 
> Do we need to forbid VLAs or do I need to refine the check?
> 
> It appears that `JumpDiagnostics.cpp` is already diagnosing statement 
> expressions and `try`. However I could not get testing to work. I tried 
> adding a test with `try`  but even with `-fexceptions` I am getting:
> 
> ```
> cannot use 'try' with exceptions disabled
> ```
> Do we need to forbid VLAs or do I need to refine the check?

Assuming that LLVM supports musttail calls from functions where a dynamic 
alloca is in scope, I think we should allow VLAs. The `musttail` documentation 
doesn't mention this, so I think its OK, and I can't think of a good reason why 
you wouldn't be able to `musttail` call due to a variably-sized frame.

Perhaps a good model would be to add a virtual function to permit asking a 
cleanup whether it's optional / skippable.

> I could not get testing to work.

You need `-fcxx-exceptions` to use `try`. At the `-cc1` level, we have 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 334985.
haberman added a comment.

- Fixed unit test by running `opt` in a separate invocation.
- Formatting fixes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp
  clang/test/Sema/attr-musttail.m

Index: clang/test/Sema/attr-musttail.m
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.m
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -verify %s
+
+void TestObjcBlock(void) {
+  void (^x)(void) = ^(void) {
+__attribute__((musttail)) return TestObjcBlock(); // expected-error{{'musttail' attribute cannot be used from Objective-C blocks}}
+  };
+  __attribute__((musttail)) return x();
+}
Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,189 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fexceptions %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();   // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;// expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member of different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is a member of different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsVoid(); // expected-error {{'musttail' attribute requires that all variables in scope are trivially destructible}}
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+int TestNonTrivialParam(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{'musttail' attribute requires that the return value, all parameters, and any temporaries created by the expression are trivially destructible and do not require ARC}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+HasNonTrivialDestructor TestReturnsNonTrivialValue() {
+  [[clang::musttail]] return (ReturnsNonTrivialValue()); // expected-error {{'musttail' attribute requires that 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added a comment.

I added tests for all the cases you mentioned. PTAL.




Comment at: clang/lib/CodeGen/CGCall.cpp:5315-5317
+// TODO(haberman): insert checks/assertions to verify that this early exit
+// is safe. We tried to verify this in Sema but we should double-check
+// here.

rsmith wrote:
> haberman wrote:
> > aaron.ballman wrote:
> > > Are you planning to handle this TODO in the patch? If not, can you switch 
> > > to a FIXME without a name associated with it?
> > I am interested in feedback on the best way to proceed here.
> > 
> >   - Is my assessment correct that we should have an assertion that 
> > validates this?
> >   - Is such an assertion reasonably feasible to implement?
> >   - Is it ok to defer with FIXME, or should I try to fix it in this patch?
> > 
> > I've changed it to a FIXME for now.
> Yes, I think we should validate this by an assertion if we can. We can check 
> this by walking the cleanup scope stack (walk from `CurrentCleanupScopeDepth` 
> to `EHScopeStack::stable_end()`) and making sure that there is no 
> "problematic" enclosing cleanup scope. Here, "problematic" would mean any 
> scope other than an `EHCleanupScope` containing only `CallLifetimeEnd` 
> cleanups.
> 
> Looking at the kinds of cleanups that we might encounter here, I think there 
> may be a few more things that Sema needs to check in order to not get in the 
> way of exception handling. In particular, I think we should reject if the 
> callee is potentially-throwing and the musttail call is inside a try block or 
> a function that's either noexcept or has a dynamic exception specification.
> 
> Oh, also, we should disallow musttail calls inside statement expressions, in 
> order to defend against cleanups that exist transiently within an expression.
I'm having trouble implementing the check because there doesn't appear to be 
any discriminator in `EHScopeStack::Cleanup` that will let you test if it is a 
`CallLifetimeEnd`. (The actual code just does virtual dispatch through 
`EHScopeStack::Cleanup::Emit()`.

I temporarily implemented this by adding an extra virtual function to act as 
discriminator. The check fires if a VLA is in scope:

```
int Func14(int x) {
  int vla[x];
  [[clang::musttail]] return Bar(x);
}
```

Do we need to forbid VLAs or do I need to refine the check?

It appears that `JumpDiagnostics.cpp` is already diagnosing statement 
expressions and `try`. However I could not get testing to work. I tried adding 
a test with `try`  but even with `-fexceptions` I am getting:

```
cannot use 'try' with exceptions disabled
```



Comment at: clang/lib/CodeGen/CGExpr.cpp:4828
  ReturnValueSlot ReturnValue) {
+  SaveAndRestore SaveMustTail(InMustTailCallExpr, E == MustTailCall);
+

rsmith wrote:
> The more I think about this, the more it makes me nervous: if any of the 
> `Emit*CallExpr` functions below incidentally emit a call on the way to 
> producing their results via the CGCall machinery, and do so without recursing 
> through this function, that incidental call will be emitted as a tail call 
> instead of the intended one. Specifically:
> 
> -- I could imagine a block call involving multiple function calls, depending 
> on the blocks ABI.
> -- I could imagine a member call performing a function call to convert from 
> derived to virtual base in some ABIs.
> -- A CUDA kernel call in general involves calling a setup function before the 
> actual function call happens (and it doesn't make sense for a CUDA kernel 
> call to be a tail call anyway...)
> -- A call to a builtin can result in any number of function calls.
> -- If any expression in the function arguments emits a call without calling 
> back into this function, we'll emit that call as a tail call instead of this 
> one. Eg, `[[clang::musttail]] return f(dynamic_cast(p));` might emit the 
> call to `__cxa_dynamic_cast` as the tail call instead of emitting the call to 
> `f` as the tail call, depending on whether the CGCall machinery is used when 
> emitting the `__cxa_dynamic_cast` call.
> 
> Is it feasible to sink this check into the `CodeGenFunction::EmitCall` 
> overload that takes a `CallExpr`, 
> `CodeGenFunction::EmitCXXMemberOrOperatorCall`, and 
> `CodeGenFunction::EmitCXXMemberPointerCallExpr`, after we've emitted the 
> callee and call args? It looks like we might be able to check this 
> immediately before calling the CGCall overload of `EmitCall`, so we could 
> pass in the 'musttail' information as a flag or similar instead of using 
> global state in the `CodeGenFunction` object; if so, it'd be much easier to 
> be confident that we're applying the attribute to the right call.
Done. It's feeling like `IsMustTail`, `callOrInvoke`, and `Loc` might want to 
get collapsed into an options struct, especially given the default parameters 
on the first two. Maybe could do as a follow up?

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 334890.
haberman marked 6 inline comments as done.
haberman added a comment.

- Addressed more comments for musttail.
- Reject constructors and destructors from musttail.
- Fixed a few bugs and fixed the tests.
- Added Obj-C test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/EHScopeStack.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp
  clang/test/Sema/attr-musttail.m

Index: clang/test/Sema/attr-musttail.m
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.m
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -fsyntax-only -fblocks -verify %s
+
+void TestObjcBlock(void) {
+  void (^x)(void) = ^(void) {
+__attribute__((musttail)) return TestObjcBlock(); // expected-error{{'musttail' attribute cannot be used from Objective-C blocks}}
+  };
+  __attribute__((musttail)) return x();
+}
Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,189 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fexceptions %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();   // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;// expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function is a member of different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function is a member of different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+  int ReturnsInt();
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsVoid(); // expected-error {{'musttail' attribute requires that all variables in scope are trivially destructible}}
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+int TestNonTrivialParam(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{'musttail' attribute requires that the return value, all parameters, and any temporaries created by the expression are trivially destructible and do not require ARC}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+HasNonTrivialDestructor TestReturnsNonTrivialValue() 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I've tried to think of some more exotic corner cases. I'd like to see a test 
for this:

  void f(); struct A { ~A() { [[clang::musttail]] return f(); } };

... even though we reject that for a reason unrelated to musttail. We should 
also reject this:

  struct B {};
  void f(B b) { [[clang::musttail]] return b.~B(); }

... because the calling convention for a destructor can be different from the 
calling convention for a regular function (eg, destructors sometimes implicitly 
return `this` and sometimes have secret extra parameters).

Please also make sure we reject

  struct A {}; void f(A a) { [[clang::musttail]] return a.A::A(); }

(which we would accept without the attribute under `-fms-extensions`): 
constructors, like destructors, have unusual calling conventions.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2845
+"target function "
+"%select{has different class%diff{ (expected $ but has $)|}1,2"
+"|has different number of parameters (expected %1 but has %2)"

Might be clearer?



Comment at: clang/lib/CodeGen/CGCall.cpp:5315-5317
+// TODO(haberman): insert checks/assertions to verify that this early exit
+// is safe. We tried to verify this in Sema but we should double-check
+// here.

haberman wrote:
> aaron.ballman wrote:
> > Are you planning to handle this TODO in the patch? If not, can you switch 
> > to a FIXME without a name associated with it?
> I am interested in feedback on the best way to proceed here.
> 
>   - Is my assessment correct that we should have an assertion that validates 
> this?
>   - Is such an assertion reasonably feasible to implement?
>   - Is it ok to defer with FIXME, or should I try to fix it in this patch?
> 
> I've changed it to a FIXME for now.
Yes, I think we should validate this by an assertion if we can. We can check 
this by walking the cleanup scope stack (walk from `CurrentCleanupScopeDepth` 
to `EHScopeStack::stable_end()`) and making sure that there is no "problematic" 
enclosing cleanup scope. Here, "problematic" would mean any scope other than an 
`EHCleanupScope` containing only `CallLifetimeEnd` cleanups.

Looking at the kinds of cleanups that we might encounter here, I think there 
may be a few more things that Sema needs to check in order to not get in the 
way of exception handling. In particular, I think we should reject if the 
callee is potentially-throwing and the musttail call is inside a try block or a 
function that's either noexcept or has a dynamic exception specification.

Oh, also, we should disallow musttail calls inside statement expressions, in 
order to defend against cleanups that exist transiently within an expression.



Comment at: clang/lib/CodeGen/CGExpr.cpp:4828
  ReturnValueSlot ReturnValue) {
+  SaveAndRestore SaveMustTail(InMustTailCallExpr, E == MustTailCall);
+

The more I think about this, the more it makes me nervous: if any of the 
`Emit*CallExpr` functions below incidentally emit a call on the way to 
producing their results via the CGCall machinery, and do so without recursing 
through this function, that incidental call will be emitted as a tail call 
instead of the intended one. Specifically:

-- I could imagine a block call involving multiple function calls, depending on 
the blocks ABI.
-- I could imagine a member call performing a function call to convert from 
derived to virtual base in some ABIs.
-- A CUDA kernel call in general involves calling a setup function before the 
actual function call happens (and it doesn't make sense for a CUDA kernel call 
to be a tail call anyway...)
-- A call to a builtin can result in any number of function calls.
-- If any expression in the function arguments emits a call without calling 
back into this function, we'll emit that call as a tail call instead of this 
one. Eg, `[[clang::musttail]] return f(dynamic_cast(p));` might emit the 
call to `__cxa_dynamic_cast` as the tail call instead of emitting the call to 
`f` as the tail call, depending on whether the CGCall machinery is used when 
emitting the `__cxa_dynamic_cast` call.

Is it feasible to sink this check into the `CodeGenFunction::EmitCall` overload 
that takes a `CallExpr`, `CodeGenFunction::EmitCXXMemberOrOperatorCall`, and 
`CodeGenFunction::EmitCXXMemberPointerCallExpr`, after we've emitted the callee 
and call args? It looks like we might be able to check this immediately before 
calling the CGCall overload of `EmitCall`, so we could pass in the 'musttail' 
information as a flag or similar instead of using global state in the 
`CodeGenFunction` object; if so, it'd be much easier to be confident that we're 
applying the attribute to the right call.



Comment at: clang/lib/Sema/SemaStmt.cpp:581
+if (EWC->cleanupsHaveSideEffects()) {
+  Diag(St->getBeginLoc(), 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman added a comment.

I addressed nearly all of the comments. I have just a few more test cases to 
add: Obj-C blocks and ARC.

I left one comment open re: a "regular" function. I'll dig into that more when 
I am adding the last few test cases.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2832
+def err_musttail_only_from_function : Error<
+  "%0 attribute can only be used from a regular function.">;
+def err_musttail_return_type_mismatch : Error<

aaron.ballman wrote:
> What is a "regular function?"
I may have been trying to distinguish between blocks, or lambdas, I can't 
exactly remember.

I think I still need to add tests for blocks and Obj-C refcounts. I'm going to 
leave this comment open for now as a reminder to revisit this.



Comment at: clang/lib/CodeGen/CGCall.cpp:5315-5317
+// TODO(haberman): insert checks/assertions to verify that this early exit
+// is safe. We tried to verify this in Sema but we should double-check
+// here.

aaron.ballman wrote:
> Are you planning to handle this TODO in the patch? If not, can you switch to 
> a FIXME without a name associated with it?
I am interested in feedback on the best way to proceed here.

  - Is my assessment correct that we should have an assertion that validates 
this?
  - Is such an assertion reasonably feasible to implement?
  - Is it ok to defer with FIXME, or should I try to fix it in this patch?

I've changed it to a FIXME for now.



Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

aaron.ballman wrote:
> This functionality belongs in SemaStmtAttr.cpp, I think.
That is where I had originally put it, but that didn't work for templates. The 
semantic checks can only be performed at instantiation time. 
`ActOnAttributedStmt` seems to be the right hook point where I can evaluate the 
semantic checks for both template and non-template functions (with template 
functions getting checked at instantiation time).



Comment at: clang/lib/Sema/SemaStmt.cpp:620
+const CXXMethodDecl *CMD = dyn_cast(mem->getMemberDecl());
+assert(CMD && !CMD->isStatic());
+CalleeThis = CMD->getThisType()->getPointeeType();

aaron.ballman wrote:
> Is this assertion valid? Consider:
> ```
> struct S {
>   static int foo();
> };
> 
> int bar() {
>   S s;
>   [[clang::musttail]] return s.foo();
> }
> ```
I have a test for that in `CodeGen/attr-musttail.cpp` (see `Func4()`). It 
appears that this goes through `FunctionToPointerDecay` and ends up getting 
handled by the function case below.



Comment at: clang/lib/Sema/SemaStmt.cpp:665-666
+ArrayRef caller_params = CallerDecl->parameters();
+size_t n = CallerDecl->param_size();
+for (size_t i = 0; i < n; i++) {
+  if (!TypesMatch(callee_params[i], caller_params[i]->getType())) {

aaron.ballman wrote:
> How do you want to handle variadic function calls?
I added a check to disallow variadic function calls.



Comment at: clang/test/Sema/attr-musttail.cpp:17
+long g(int x);
+int h(long x);
+

aaron.ballman wrote:
> I'd appreciate a test of promotion behavior:
> ```
> int i(int x);
> int s(short x);
> 
> int whatever1(int x) {
>   [[clang::musttail]] return s(x);
> }
> 
> int whatever2(short x) {
>   [[clang::musttail]] return i(x);
> }
> ```
Done (my understanding is that these should both fail, since they would violate 
the LLVM rules that the caller and callee prototypes must match).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 334556.
haberman marked 37 inline comments as done.
haberman added a comment.

- Expanded and refined the semantic checks for musttail, per CR feedback.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.c
  clang/test/Sema/attr-musttail.cpp

Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+int ReturnsInt1();
+int Func1() {
+  [[clang::musttail]] ReturnsInt1();   // expected-error {{'musttail' attribute only applies to return statements}}
+  [[clang::musttail(1, 2)]] return ReturnsInt1(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] return 5;// expected-error {{'musttail' attribute requires that the return value is the result of a function call}}
+  [[clang::musttail]] return ReturnsInt1();
+}
+
+[[clang::musttail]] static int int_val = ReturnsInt1(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+void NoParams(); // expected-note {{target function has different number of parameters (expected 1 but has 0)}}
+void TestParamArityMismatch(int x) {
+  [[clang::musttail]] return NoParams(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void LongParam(long x); // expected-note {{target function has type mismatch at 1st parameter (expected 'long' but has 'int')}}
+void TestParamTypeMismatch(int x) {
+  [[clang::musttail]] return LongParam(x); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+long ReturnsLong(); // expected-note {{target function has different return type ('int' expected but has 'long')}}
+int TestReturnTypeMismatch() {
+  [[clang::musttail]] return ReturnsLong(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+struct Struct1 {
+  void MemberFunction(); // expected-note {{target function has different class (expected 'void' but has 'Struct1')}}
+};
+void TestNonMemberToMember() {
+  Struct1 st;
+  [[clang::musttail]] return st.MemberFunction(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}}
+}
+
+void ReturnsVoid(); // expected-note {{target function has different class (expected 'Struct2' but has 'void')}}
+struct Struct2 {
+  void TestMemberToNonMember() {
+[[clang::musttail]] return ReturnsVoid(); // expected-error{{'musttail' attribute requires that caller and callee have compatible function signatures}}
+  }
+};
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() {}
+};
+
+void ReturnsVoid2();
+void TestNonTrivialDestructorInScope() {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsVoid(); // expected-error {{'musttail' attribute requires that all variables in scope are trivially destructible}}
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+int TestNonTrivialParam(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{'musttail' attribute requires that the return type and all arguments are trivially destructible}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+HasNonTrivialDestructor TestReturnsNonTrivialValue() {
+  [[clang::musttail]] return ReturnsNonTrivialValue(); // expected-error {{'musttail' attribute requires that the return type and all arguments are trivially destructible}}
+}
+
+struct UsesPointerToMember {
+  void (UsesPointerToMember::*p_mem)();
+};
+void TestUsesPointerToMember(UsesPointerToMember *foo) {
+  // "this" pointer cannot double as first parameter.
+  [[clang::musttail]] return (foo->*(foo->p_mem))(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}} expected-note{{target function has different class (expected 'void' but has 'UsesPointerToMember')}}
+}
+
+void ReturnsVoid2();
+void TestNestedClass() {
+  HasNonTrivialDestructor foo;
+  class Nested {
+__attribute__((noinline)) static void NestedMethod() {
+  // Outer non-trivial 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2829-2830
+def err_musttail_needs_call : Error<
+  "%0 attribute requires that the return value is a function call, which must "
+  "not create or destroy any temporaries.">;
+def err_musttail_only_from_function : Error<

aaron.ballman wrote:
> 
Can we diagnose these two cases separately?



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2833-2835
+def err_musttail_return_type_mismatch : Error<
+  "%0 attribute requires that caller and callee have identical parameter types 
"
+  "and return types">;

It would be useful to say what didn't match. Eg, parameter index or parameter 
name. 



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2837
+def err_musttail_no_destruction : Error<
+  "%0 attribute does not allow any variables in scope that require destruction"
+  >;

aaron.ballman wrote:
> All automatic variables require destruction when leaving the scope, so this 
> diagnostic reads oddly to me. Perhaps you mean that the variables must all be 
> trivially destructible?
Can we say which variable? I think it'd be useful to have both a diagnostic and 
a note here, pointing to the attribute and variable.



Comment at: clang/lib/CodeGen/CGCall.cpp:5313
+  // If this is a musttail call, return immediately. We do not branch to the
+  // prologue in this case.
+  if (InMustTailCallExpr) {

epilogue?



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:35
+/// cleanups properly.  Indirect jumps and ASM jumps can't do cleanups because
+/// the target is unknown.  Return statements with \c [musttail] cannot handle
+/// any cleanups due to the nature of a tail call.





Comment at: clang/lib/Sema/SemaStmtAttr.cpp:15
 #include "clang/AST/EvaluatedExprVisitor.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/Basic/SourceManager.h"

Looks like you're not using this. (And that's good: the parent map should never 
be used in the static compiler; it's a tooling-only facility.)



Comment at: clang/test/CodeGen/attr-musttail.cpp:8
+  if (x) {
+[[clang::musttail]] return Bar(x); // CHECK: %call = musttail call i32 
@_Z3Bari(i32 %1)
+  } else {

I'd like to see a `CHECK-NEXT: ret i32 %call` here too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1356
+def MustTail : StmtAttr {
+  let Spellings = [Clang<"musttail">];
+  let Documentation = [MustTailDocs];

You should add a `Subjects` list here.



Comment at: clang/include/clang/Basic/AttrDocs.td:449
+  let Content = [{
+If a return statement is marked ``musttail``, this indicates that the
+compiler must generate a tail call for the program to be correct, even when





Comment at: clang/include/clang/Basic/AttrDocs.td:454
+
+``clang::musttail`` can only be applied to a return statement whose value is a
+function call (even functions returning void must use 'return', although no





Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2826-2827
   InGroup;
+def err_musttail_only_on_return : Error<
+  "%0 attribute can only be applied to a return statement">;
+def err_musttail_needs_call : Error<

This error should not be necessary if you add the correct subject line to 
Attr.td



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2829-2830
+def err_musttail_needs_call : Error<
+  "%0 attribute requires that the return value is a function call, which must "
+  "not create or destroy any temporaries.">;
+def err_musttail_only_from_function : Error<





Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2832
+def err_musttail_only_from_function : Error<
+  "%0 attribute can only be used from a regular function.">;
+def err_musttail_return_type_mismatch : Error<

What is a "regular function?"



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2837
+def err_musttail_no_destruction : Error<
+  "%0 attribute does not allow any variables in scope that require destruction"
+  >;

All automatic variables require destruction when leaving the scope, so this 
diagnostic reads oddly to me. Perhaps you mean that the variables must all be 
trivially destructible?



Comment at: clang/include/clang/Sema/ScopeInfo.h:121
 
+  /// Whether this function contains any statement marked with \c [musttail].
+  bool HasMustTail : 1;





Comment at: clang/lib/CodeGen/CGCall.cpp:5315-5317
+// TODO(haberman): insert checks/assertions to verify that this early exit
+// is safe. We tried to verify this in Sema but we should double-check
+// here.

Are you planning to handle this TODO in the patch? If not, can you switch to a 
FIXME without a name associated with it?



Comment at: clang/lib/CodeGen/CGCall.cpp:5318-5322
+if (RetTy->isVoidType()) {
+  Builder.CreateRetVoid();
+} else {
+  Builder.CreateRet(CI);
+}





Comment at: clang/lib/CodeGen/CGExpr.cpp:4828
  ReturnValueSlot ReturnValue) {
+  SaveAndRestore save_musttail(InMustTailCallExpr, E == MustTailCall);
+





Comment at: clang/lib/CodeGen/CGStmt.cpp:655-658
+  const ReturnStmt *R = dyn_cast(Sub);
+  assert(R && "musttail should only be on ReturnStmt");
+  musttail = dyn_cast(R->getRetValue());
+  assert(musttail && "musttail must return CallExpr");





Comment at: clang/lib/Sema/JumpDiagnostics.cpp:1005-1011
+  for (const auto *A : AS->getAttrs()) {
+if (A->getKind() == attr::MustTail) {
+  return A;
+}
+  }
+
+  return nullptr;





Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+if (A->getKind() == attr::MustTail) {
+  if (!checkMustTailAttr(SubStmt, *A)) {
+return SubStmt;
+  }
+  setFunctionHasMustTail();
+}

This functionality belongs in SemaStmtAttr.cpp, I think.



Comment at: clang/lib/Sema/SemaStmt.cpp:578-582
+  if (!R) {
+Diag(St->getBeginLoc(), diag::err_musttail_only_on_return)
+<< MTA.getSpelling();
+return false;
+  }

This check should not be necessary once the code moves to SemaStmtAttr.cpp and 
Attr.td gets a correct subjects line.



Comment at: clang/lib/Sema/SemaStmt.cpp:584-591
+  const Expr *Ex = R->getRetValue();
+
+  // We don't actually support tail calling through an implicit cast (we 
require
+  // the return types to match), but getting the actual function call will let
+  // us give a better error message about the return type mismatch.
+  if (const ImplicitCastExpr *ICE = dyn_cast(Ex)) {
+Ex = ICE->getSubExpr();

I think you want to ignore parens and implicit casts here. e.g., there's no 
reason to diagnose code like:
```
int foo();
int bar() {
  [[clang::musttail]] return (bar());
}
```



Comment at: 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman updated this revision to Diff 333914.
haberman added a comment.

- Updated formatting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99517/new/

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.cpp

Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,137 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+int Bar();
+
+int Func1() {
+  [[clang::musttail(1, 2)]] Bar(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] Bar();   // expected-error {{musttail attribute can only be applied to a return statement}}
+  [[clang::musttail]] return 5;// expected-error {{musttail attribute requires that the return value is a function call}}
+  [[clang::musttail]] return Bar();
+}
+
+int f();
+
+[[clang::musttail]] static int i = f(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+long g(int x);
+int h(long x);
+
+class Foo {
+public:
+  int MemberFunction(int x);
+  int MemberFunction2();
+};
+
+int Func2(int x) {
+  // Param arity differs.
+  [[clang::musttail]] return Bar(); // expected-error {{musttail attribute requires that caller and callee have identical parameter types and return types}}
+  // Return type differs.
+  [[clang::musttail]] return g(x); // expected-error {{musttail attribute requires that caller and callee have identical parameter types and return types}}
+  // Param type differs.
+  [[clang::musttail]] return h(x); // expected-error {{musttail attribute requires that caller and callee have identical parameter types and return types}}
+  // "this" pointer differs.
+  Foo foo;
+  [[clang::musttail]] return foo.MemberFunction(x); // expected-error {{musttail attribute requires that caller and callee have identical parameter types and return types}}
+}
+
+int j = 0;
+
+class HasNonTrivialDestructor {
+public:
+  ~HasNonTrivialDestructor() { j--; }
+};
+
+int ReturnsInt(int x);
+
+int Func3(int x) {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsInt(x); // expected-error {{musttail attribute does not allow any variables in scope that require destruction}}
+}
+
+int Func4(int x) {
+  HasNonTrivialDestructor foo; // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  {
+[[clang::musttail]] return ReturnsInt(x); // expected-error {{musttail attribute does not allow any variables in scope that require destruction}}
+  }
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+
+int Func5(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x); // expected-error {{musttail attribute requires that the return value is a function call, which must not create or destroy any temporaries}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+
+HasNonTrivialDestructor Func6() {
+  [[clang::musttail]] return ReturnsNonTrivialValue(); // expected-error {{musttail attribute requires that the return value is a function call, which must not create or destroy any temporaries}}
+}
+
+int Func8(Foo *foo, int (Foo::*p_mem)()) {
+  [[clang::musttail]] return (foo->*p_mem)(); // expected-error {{musttail attribute requires that caller and callee have identical parameter types and return types}}
+}
+
+int Func10(Foo *foo) {
+  [[clang::musttail]] return foo->MemberFunction2(); // expected-error {{musttail attribute requires that caller and callee have identical parameter types and return types}}
+}
+
+void Func7() {
+  HasNonTrivialDestructor foo;
+  class Nested {
+__attribute__((noinline)) static int NestedMethod(int x) {
+  // This is ok.
+  [[clang::musttail]] return ReturnsInt(x);
+}
+  };
+}
+
+struct Data {
+  int (Data::*pmf)();
+  typedef int Func(Data *);
+  static void StaticMethod();
+  void NonStaticMethod() {
+[[clang::musttail]] return StaticMethod(); // expected-error {{musttail attribute requires that caller and callee have identical parameter types and return types}}
+  }
+
+  void BadPMF() {
+// We need to specially handle this, otherwise it can crash the compiler.
+[[clang::musttail]] return ((*this)->*pmf)(); // expected-error {{left hand operand to ->* must be a pointer to class compatible with the right hand operand, but is 'Data'}}
+  }
+};
+
+Data 

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Josh Haberman via Phabricator via cfe-commits]

haberman created this revision.
haberman added reviewers: rsmith, aaron.ballman.
haberman requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is a Clang-only change and depends on the existing "musttail"
support already implemented in LLVM.

The [[clang::musttail]] attribute goes on a return statement, not
a function definition. There are several constraints that the user
must follow when using [[clang::musttail]], and these constraints
are verified by Sema.

Tail calls are supported on regular function calls, calls through a
function pointer, member function calls, and even pointer to member.

Future work would be to throw a warning if a users tries to pass
a pointer or reference to a local variable through a musttail call.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D99517

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-musttail.cpp
  clang/test/Sema/attr-musttail.cpp

Index: clang/test/Sema/attr-musttail.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-musttail.cpp
@@ -0,0 +1,137 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+int Bar();
+
+int Func1() {
+  [[clang::musttail(1, 2)]] Bar(); // expected-error {{'musttail' attribute takes no arguments}}
+  [[clang::musttail]] Bar(); // expected-error {{musttail attribute can only be applied to a return statement}}
+  [[clang::musttail]] return 5; // expected-error {{musttail attribute requires that the return value is a function call}}
+  [[clang::musttail]] return Bar();
+}
+
+int f();
+
+[[clang::musttail]] static int i = f(); // expected-error {{'musttail' attribute cannot be applied to a declaration}}
+
+long g(int x);
+int h(long x);
+
+class Foo {
+ public:
+  int MemberFunction(int x);
+  int MemberFunction2();
+};
+
+int Func2(int x) {
+  // Param arity differs.
+  [[clang::musttail]] return Bar(); // expected-error {{musttail attribute requires that caller and callee have identical parameter types and return types}}
+  // Return type differs.
+  [[clang::musttail]] return g(x); // expected-error {{musttail attribute requires that caller and callee have identical parameter types and return types}}
+  // Param type differs.
+  [[clang::musttail]] return h(x); // expected-error {{musttail attribute requires that caller and callee have identical parameter types and return types}}
+  // "this" pointer differs.
+  Foo foo;
+  [[clang::musttail]] return foo.MemberFunction(x); // expected-error {{musttail attribute requires that caller and callee have identical parameter types and return types}}
+}
+
+int j = 0;
+
+class HasNonTrivialDestructor {
+ public:
+  ~HasNonTrivialDestructor() { j--; }
+};
+
+int ReturnsInt(int x);
+
+int Func3(int x) {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  [[clang::musttail]] return ReturnsInt(x);  // expected-error {{musttail attribute does not allow any variables in scope that require destruction}}
+}
+
+int Func4(int x) {
+  HasNonTrivialDestructor foo;  // expected-note {{jump exits scope of variable with non-trivial destructor}}
+  {
+[[clang::musttail]] return ReturnsInt(x);  // expected-error {{musttail attribute does not allow any variables in scope that require destruction}}
+  }
+}
+
+int NonTrivialParam(HasNonTrivialDestructor x);
+
+int Func5(HasNonTrivialDestructor x) {
+  [[clang::musttail]] return NonTrivialParam(x);  // expected-error {{musttail attribute requires that the return value is a function call, which must not create or destroy any temporaries}}
+}
+
+HasNonTrivialDestructor ReturnsNonTrivialValue();
+
+HasNonTrivialDestructor Func6() {
+  [[clang::musttail]] return ReturnsNonTrivialValue();  // expected-error {{musttail attribute requires that the return value is a function call, which must not create or destroy any temporaries}}
+}
+
+int Func8(Foo* foo, int (Foo::*p_mem)()) {
+  [[clang::musttail]] return (foo->*p_mem)(); // expected-error {{musttail attribute requires that caller and callee have identical parameter types and return types}}
+}
+
+int Func10(Foo* foo) {
+  [[clang::musttail]] return foo->MemberFunction2(); // expected-error {{musttail attribute requires that caller and callee have identical parameter types and return types}}
+}
+
+void Func7() {
+  HasNonTrivialDestructor foo;
+  class Nested {
+__attribute__((noinline)) static int NestedMethod(int x) {
+  // This is ok.
+  [[clang::musttail]] return