[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<bool> 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<T*>(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<const ReturnStmt>(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<CapturedDecl>(CurContext)) {
+    int ContextType;
----------------
I think this `isa<CapturedDecl>` 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