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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks! This looks like exactly the right way to compute when to apply NRVO. It'd be good to track (in your commit message at least) that this addresses PR13067.



================
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());
----------------
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.)


================
Comment at: test/CodeGenCXX/nrvo.cpp:139
   }
-  // FIXME: we should NRVO this variable too.
-  X x;
+  // CHECK: tail call {{.*}} @_ZN1XC1ERKS_
   return x;
----------------
tzik wrote:
> Quuxplusone wrote:
> > You've changed this function from testing one thing with a FIXME, to testing a completely different thing.  Could you add your new code as a new function, and leave the old FIXME alone until it's fixed?
> > Alternatively, if your patch actually fixes the FIXME, could you just replace the FIXME comment with a CHECK and leave the rest of this function alone?
> My patch fixes the FIXME.
> However, on the resulting code of NRVOing,
> ```
> X y;
> return y;
> ```
> and
> ```
> X x;
> return x;
> ```
> get to the same code and unified. And, the function is simplified as
> ```
> X test3(bool B) {
>   X x;
>   return x;
> }
> ```
> without the if statement.
> So, there will be nothing to CHECK left here if I leave the code as-is. I think that does not fit to the test scenario.
... and this is one reason why we don't generally like clang's IR generation tests to enable optimizations. Please consider starting a new test file to test the output from clang's frontend rather than the output from -O1.


Repository:
  rC Clang

https://reviews.llvm.org/D47067