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">;
+def err_musttail_needs_call : Error<
----------------
rsmith wrote:
> Can we somehow avoid talking about ARC where it's not relevant? While it'd be
> nice to be more precise here, my main concern is that we shouldn't be
> mentioning ARC to people for whom it's not a meaningful term (eg, when not
> compiling Objective-C or Objective-C++). Perhaps the simplest approach would
> be to only mention ARC if `getLangOpts().ObjCAutoRefCount` is set?
I implemented this but I couldn't figure out how to actually trigger the ARC
case, so I just removed that part of the diagnostic text for now.
================
Comment at: clang/lib/AST/AttrImpl.cpp:221-226
+ // Elide constructors even if this is disabled with -fno-elide-constructors
+ // because 'musttail' is a more local setting.
+ if (CCE && CCE->isElidable()) {
+ assert(CCE->getNumArgs() == 1); // Copy or move constructor.
+ Ex = CCE->getArg(0);
+ }
----------------
rsmith wrote:
> `IgnoreImplicitAsWritten` should already skip over implicit elidable
> constructors, so I would imagine this is skipping over elidable //explicit//
> constructor calls (eg, `[[musttail]] return T(make());` would perform a
> tail-call to `make()`). Is that what we want?
As discussed offline, it appears that `IgnoreImplicitAsWritten()` was not
skipping the implicit constructor in this case. Per our discussion, I created a
new version of `IgnoreImplicitAsWritten()` that does, with a FIXME to land it
in `Expr`, and I made it skip implicit constructors only (and added tests for
this case).
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:665
+ SaveAndRestore<const CallExpr *> save_musttail(MustTailCall, musttail);
EmitStmt(S.getSubStmt(), S.getAttrs());
}
----------------
rsmith wrote:
> In the case where we're forcibly eliding a constructor, we'll need to emit a
> return statement that returns `musttail` call expression here rather than
> emitting the original substatement. Otherwise the tail call we emit will be
> initializing a local temporary rather than initializing our return slot. Eg,
> given:
>
> ```
> struct A {
> A(const A&);
> ~A();
> char data[32];
> };
> A f();
> A g() {
> [[clang::musttail]] return f();
> }
> ```
> under `-fno-elide-constructors` when targeting C++11, say, we'll normally
> lower that into something like:
> ```
> void f(A *return_slot);
> void g(A *return_slot) {
> A temporary; //uninitialized
> f(&temporary); // call f
> A::A(return_slot, temporary); // call copy constructor to copy into return
> slot
> }
> ```
> ... and with the current patch, it looks like we'll add a 'ret void' after
> the call to `f`, leaving `g`'s return slot uninitialized and passing an
> address into `f` that refers to a variable that will no longer exist once `f`
> is called. We need to instead lower to:
> ```
> void f(A *return_slot);
> void g(A *return_slot) {
> f(return_slot); // call f
> }
> ```
> Probably the easiest way to do this would be to change the return value on
> the `ReturnStmt` to be the tail-called `CallExpr` when attaching the
> attribute.
Done.
I had to change your test case to remove the destructor, otherwise it fails the
trivial destruction check.
Take a look at the CodeGen tests and see if the output looks correct to you.
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
