[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<IgnoredAttributes>;
+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<bool> save_musttail(InMustTailCallExpr, E == MustTailCall);
+
----------------



================
Comment at: clang/lib/CodeGen/CGStmt.cpp:655-658
+      const ReturnStmt *R = dyn_cast<ReturnStmt>(Sub);
+      assert(R && "musttail should only be on ReturnStmt");
+      musttail = dyn_cast<CallExpr>(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<ImplicitCastExpr>(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: clang/lib/Sema/SemaStmt.cpp:596
+  if (!CE) {
+    Diag(St->getBeginLoc(), diag::err_musttail_needs_call) << MTA.getSpelling();
+    return false;
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:607
+    Diag(St->getBeginLoc(), diag::err_musttail_only_from_function)
+        << MTA.getSpelling();
+    return false;
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:609-612
+  } else if (CallerDecl->isDependentContext()) {
+    // We have to suspend our check until template instantiation time.
+    return true;
+  }
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:617
+  // type of "this".
+  if (const MemberExpr *mem = dyn_cast<MemberExpr>(Callee)) {
+    // Call is: obj.method() or obj->method()
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:620
+    const CXXMethodDecl *CMD = dyn_cast<CXXMethodDecl>(mem->getMemberDecl());
+    assert(CMD && !CMD->isStatic());
+    CalleeThis = CMD->getThisType()->getPointeeType();
----------------
Is this assertion valid? Consider:
```
struct S {
  static int foo();
};

int bar() {
  S s;
  [[clang::musttail]] return s.foo();
}
```


================
Comment at: clang/lib/Sema/SemaStmt.cpp:640-641
+    }
+    CalleeType = FunctionType->getUnqualifiedDesugaredType()
+                     ->castAs<FunctionProtoType>();
+  }
----------------
I think this could assert on K&R C declarations because those don't have a prototype. e.g.,
```
int f(); // Important: compile this as C code, not C++ code

int bar(void) {
  [[clang::musttail]] return f();
}
```


================
Comment at: clang/lib/Sema/SemaStmt.cpp:650-656
+  auto TypesMatch = [this](QualType a, QualType b) -> bool {
+    if (a == QualType() || b == QualType()) {
+      return a == b;
+    } else {
+      return Context.hasSimilarType(a, b);
+    }
+  };
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:658
+
+  bool types_match =
+      TypesMatch(CallerDecl->getReturnType(), CalleeType->getReturnType()) &&
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:663-664
+  if (types_match) {
+    ArrayRef<QualType> callee_params = CalleeType->getParamTypes();
+    ArrayRef<ParmVarDecl *> caller_params = CallerDecl->parameters();
+    size_t n = CallerDecl->param_size();
----------------



================
Comment at: clang/lib/Sema/SemaStmt.cpp:665-666
+    ArrayRef<ParmVarDecl *> 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())) {
----------------
How do you want to handle variadic function calls?


================
Comment at: clang/lib/Sema/SemaStmt.cpp:676
+    Diag(St->getBeginLoc(), diag::err_musttail_return_type_mismatch)
+        << MTA.getSpelling();
+    return false;
----------------



================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:215-217
+  MustTailAttr MTA(S.Context, A);
+  if (S.CheckAttrNoArgs(A))
+    return nullptr;
----------------
None of this should be needed due to the automated diagnostic checking.


================
Comment at: clang/test/CodeGen/attr-musttail.cpp:69
+}
+
+// CHECK: musttail call void @_Z11ReturnsVoidv()
----------------
How about something like:
```
int FuncX() {
  [[clang::musttail]] return []() { return 12; }();
}
```


================
Comment at: clang/test/Sema/attr-musttail.cpp:17
+long g(int x);
+int h(long x);
+
----------------
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);
}
```


================
Comment at: clang/test/Sema/attr-musttail.cpp:137
+  [[clang::musttail]] return ObjParam(obj); // expected-error {{musttail attribute requires that the return value is a function call, which must not create or destroy any temporaries}}
+}
----------------
Still to be tested: using a lambda, behavior in C (esp with K&R C functions), and variadic functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99517