[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