[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added a comment. @theraven: Can you post a minimal repro of your case? I don't follow your distinction between "caller" and "enclosing function." Regarding `noreturn` and `always_inline`: maybe the rules for `musttail` could be relaxed in cases like the one you mention, but it would

[PATCH] D99983: Provide TreeTransform::TransformAttr the transformed statement; NFC

haberman accepted this revision. haberman added a comment. This revision is now accepted and ready to land. This seems fine to me, but I'll defer to @rsmith for final review. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99983/new/ https://reviews.llvm.org/D99983

[PATCH] D98889: [clang] Replaced some manual pointer tagging with llvm::PointerIntPair.

haberman updated this revision to Diff 332405. haberman added a comment. Updated change to reflect all diffs since main. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98889/new/ https://reviews.llvm.org/D98889 Files:

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 334890. haberman marked 6 inline comments as done. haberman added a comment. - Addressed more comments for musttail. - Reject constructors and destructors from musttail. - Fixed a few bugs and fixed the tests. - Added Obj-C test. Repository: rG LLVM

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added a comment. I added tests for all the cases you mentioned. PTAL. 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

[PATCH] D99896: Rework the way statement attributes are processed; NFC

haberman added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:1316 Stmt *SubStmt) { -return SemaRef.ActOnAttributedStmt(AttrLoc, Attrs, SubStmt); +return SemaRef.BuildAttributedStmt(AttrLoc, Attrs, SubStmt); }

[PATCH] D99896: Rework the way statement attributes are processed; NFC

haberman added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:1316 Stmt *SubStmt) { -return SemaRef.ActOnAttributedStmt(AttrLoc, Attrs, SubStmt); +return SemaRef.BuildAttributedStmt(AttrLoc, Attrs, SubStmt); }

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added inline comments. 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(); +}

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 335106. haberman added a comment. - Added missing S.setFunctionHasMustTail(). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files:

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added inline comments. 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(); +}

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 335103. haberman marked 3 inline comments as done. haberman added a comment. - Addressed comments and tried moving check to SemaStmtAttr.cpp. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:588 + + const CallExpr *CE = dyn_cast(Ex->IgnoreUnlessSpelledInSource()); + rsmith wrote: > `IgnoreUnlessSpelledInSource` is a syntactic check that's only really > intended for tooling use

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added inline comments. 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(); +}

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 334985. haberman added a comment. - Fixed unit test by running `opt` in a separate invocation. - Formatting fixes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 333914. haberman added a comment. - Updated formatting. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files: clang/include/clang/Basic/Attr.td

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman created this revision. haberman added reviewers: rsmith, aaron.ballman. haberman requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This is a Clang-only change and depends on the existing "musttail" support already implemented in

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 334556. haberman marked 37 inline comments as done. haberman added a comment. - Expanded and refined the semantic checks for musttail, per CR feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added a comment. I addressed nearly all of the comments. I have just a few more test cases to add: Obj-C blocks and ARC. I left one comment open re: a "regular" function. I'll dig into that more when I am adding the last few test cases. Comment at:

[PATCH] D98893: Updated comment "the low bit" -> "the integer".

haberman created this revision. haberman requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D98893 Files: clang/include/clang/Sema/Initialization.h Index:

[PATCH] D98889: [clang] Replaced some manual pointer tagging with llvm::PointerIntPair.

haberman marked an inline comment as done. haberman added a comment. > Thanks! Do we need InitializedEntity::getDecl to return a pointer to > non-const? Yes, if I try to propagate `const` the change starts to snowball. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D98889: [clang] Replaced some manual pointer tagging with llvm::PointerIntPair.

haberman updated this revision to Diff 331679. haberman added a comment. Updated comment "the low bit" -> "the integer". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98889/new/ https://reviews.llvm.org/D98889 Files:

[PATCH] D98893: Updated comment "the low bit" -> "the integer".

haberman abandoned this revision. haberman added a comment. This was meant to be an update on https://reviews.llvm.org/D98889 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98893/new/ https://reviews.llvm.org/D98893

[PATCH] D98889: [clang] Replaced some manual pointer tagging with llvm::PointerIntPair.

haberman created this revision. haberman added a reviewer: rsmith. haberman requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. There is no functional change here (hence no new tests). The only change is to replace a couple uintptr_t members

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 336141. haberman added a comment. - Factored duplicated code into a method on MustTailAttr. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files:

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added a comment. In D99517#2667025 , @rjmccall wrote: > 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

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 336153. haberman added a comment. - Moved calling convention check to happen as early as possible. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files:

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added inline comments. 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(); +}

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 336203. haberman added a comment. - Formatted files with clang-format. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files: clang/include/clang/AST/Expr.h

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman marked 2 inline comments as done. haberman added inline comments. 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; + }

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 336310. haberman marked 3 inline comments as done. haberman added a comment. - Refined the implicit constructor skipping code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2828-2829 + "%0 attribute requires that the return value, all parameters, and any " + "temporaries created by the expression are trivially destructible and " + "do not require ARC">;

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 336316. haberman added a comment. - Rename and refine IgnoreElidableImplicitConstructorSingleStep(). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files:

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:458 +same number of arguments as the caller. The types of the return value and all +arguments must be similar, including the implicit "this" argument, if any. +Any variables in scope, including all

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 336004. haberman marked 19 inline comments as done. haberman added a comment. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. - Returned validation to ActOnAttributedStmt() so it works with templates. - Merge branch 'main' into

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 337597. haberman added a comment. - Fixed typo in comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files: clang/docs/ReleaseNotes.rst

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 337590. haberman added a comment. - Fixed release note escaping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files: clang/docs/ReleaseNotes.rst

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added a comment. Ok I think this is ready to land. There are a few FIXME comments, I will follow up with some small changes to address them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 337589. haberman added a comment. - Added release note for [[clang::musttail]]. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files: clang/docs/ReleaseNotes.rst

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 337592. haberman added a comment. - Fixed several cases in CodeGen test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files: clang/docs/ReleaseNotes.rst

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman marked an inline comment as done. haberman added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:697-699 + } else if (VD && dyn_cast(VD->getType())) { +// Call is: obj->*method_ptr or obj.*method_ptr +const auto *MPT = VD->getType()->castAs();

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 336894. haberman added a comment. - Switch to isa<> for type check. - Merge branch 'main' into musttail Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files:

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:605-609 + while (!isa(Ex)) { +auto *PE = cast(Ex); +Ex = IgnoreImplicitAsWritten(PE->getSubExpr()); +PE->setSubExpr(Ex); + } rsmith wrote: > This loop is problematic: it's

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 337252. haberman marked 14 inline comments as done. haberman added a comment. - Addressed more review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files:

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:711 +if (CalleeQualType.isNull()) { + // The function callee is invalid and already triggered an error. + // Avoid compounding errors. rsmith wrote: > Even in invalid code we

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 336511. haberman marked 9 inline comments as done. haberman added a comment. - Simplified some casts and type declarations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:636-637 + + if (!CE->getCalleeDecl()) { +assert(hasUncompilableErrorOccurred() && "expected previous error"); +return false; aaron.ballman wrote: > This worries me slightly -- not

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 337576. haberman marked 6 inline comments as done. haberman added a comment. - Word-smithed diagnostics and addressed other review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 337581. haberman added a comment. - More diagnostic wordsmithing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files: clang/include/clang/AST/IgnoreExpr.h

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

haberman updated this revision to Diff 336130. haberman added a comment. - Added FIXME for attribute refactoring. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517 Files: clang/include/clang/AST/Expr.h

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

haberman added a comment. > is a [[should_tail]] attribute sort of thing: a tail-hint where we do 'best > effort with no promises', and make no guarantees that we're going to tail it. I'm not sure I see the value in that. The compiler already optimizes tail calls when it can in a best-effort

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

haberman added a comment. `[[nonportable_musttail]]` makes sense to me as a semantic. It indicates that the algorithm requires tail calls, but the author is willing to accept that the algorithm may be non-portable. "Non-portable" here can mean architecture-specific, but it can also mean

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

haberman added a comment. Correct me if I am wrong, but won't this produce LLVM IR that fails to validate? The checks that exist today for `[[clang::musttail]]` are necessary to follow LLVM's rules about where `musttail` may appear: https://llvm.org/docs/LangRef.html#id328 > Calls marked

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

haberman added a comment. > Such a definition doesn't make much sense because for some targets tail call > optimization isn't available at all. > Eg. Wasm w/o tail call proposal, xtensa windowed abi. That is true. But what is the correct behavior if you try to use `[[clang::musttail]]` on such

[PATCH] D147714: [Attr] Introduce [[clang::nonportable_musttail]] as less strict version of [[clang::musttail]]

haberman added a comment. I think I misunderstood @yamt's comment in https://reviews.llvm.org/D147714#4446780. I take back what I wrote above. I agree that `[[clang::nonportable_musttail]]` is a nicer semantic, and the restrictions around the existing `[[clang:musttail]]` don't seem to buy us