[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<EHCleanupScope>(&*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<BlockDecl>(CurContext) && "unexpected decl context");
+    Diag(St->getBeginLoc(), diag::err_musttail_from_block_forbidden) << &MTA;
----------------
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