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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
