[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<const CallExpr *> 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(&temporary); // 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