[PATCH] D47067: Update NRVO logic to support early return

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: lib/Sema/SemaDecl.cpp:12760
       // to deduce an implicit return type.
-      if (FD->getReturnType()->isRecordType() &&
-          (!getLangOpts().CPlusPlus || !FD->isDependentContext()))
+      if (!FD->getReturnType()->isScalarType())
         computeNRVO(Body, getCurFunction());
----------------
rsmith wrote:
> tzik wrote:
> > Quuxplusone wrote:
> > > What is the purpose of this change?
> > > If it's "no functional change" it should be done separately IMHO; if it is supposed to change codegen, then it needs some codegen tests. (From looking at the code: maybe this affects codegen on functions that return member function pointers by value?)
> > I think the previous implementation was incorrect.
> > Though computeNRVO clears ReturnStmt::NRVOCandidate when the corresponding variable is not NRVO variable, CGStmt checks both of ReturnStmt::NRVOCandidate and VarDecl::NRVOVariable anyway.
> > So, computeNRVO took no effect in the previous code, and the absence of computeNRVO here on function templates did not matter.
> > Note that there was no chance to call computeNRVO on function template elsewhere too.
> > 
> > OTOH in the new implementation, computeNRVO is necessary, and its absence on function templates matters.
> > 
> > We can remove computeNRVO here separately as a NFC patch and readd the new impl to the same place, but it's probably less readable, IMO.
> What happens if we can't tell whether we have an NRVO candidate or not until instantiation:
> 
> ```
> template<typename T> X f() {
>   T v;
>   return v; // is an NRVO candidate if T is X, otherwise is not
> }
> ```
> 
> (I think this is not hard to handle: the dependent construct here can only affect whether you get NRVO at all, not which variable you perform NRVO on, but I want to check that it is handled properly.)
Ah, so if I understand correctly — which I probably don't —...
the parts of this diff related to `isRecordType()` make NRVO more reliable on template functions? Because the existing code has lots of checks for `isRecordType()` which means that they don't run for dependent (as-yet-unknown) types, even if those types are going to turn out to be record types occasionally?

I see that line 12838 runs `computeNRVO` unconditionally for *all* ObjCMethodDecls, //even// ones with scalar return types. So maybe running `computeNRVO` unconditionally right here would also be correct?
Datapoint: I made a patch to do nothing but replace this condition with `if (Body)` and to remove the similar condition guarding `computeNRVO` in `Sema::ActOnBlockStmtExpr`. It passed the Clang test suite with flying colors. I don't know if this means that the test suite is missing some tests, or if it means that the conditions are unnecessary.

IMHO if you're changing this anyway, it would be nice to put the remaining condition/optimization `if (getReturnType().isScalarType()) return;` inside `computeNRVO` itself, instead of repeating that condition at every call site.


Repository:
  rC Clang

https://reviews.llvm.org/D47067