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