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

[Taiju Tsuiki via Phabricator via cfe-commits]

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


================
Comment at: test/CodeGenCXX/nrvo.cpp:139
   }
-  // FIXME: we should NRVO this variable too.
-  X x;
+  // CHECK: tail call {{.*}} @_ZN1XC1ERKS_
   return x;
----------------
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.


================
Comment at: test/CodeGenCXX/nrvo.cpp:254
+  T t;
+  return t;
+}
----------------
Quuxplusone wrote:
> Just for my own curiosity: this new test case is surely unaffected by this patch, right?
Hm, this is not so important anymore. This was to check if NRVO is working with function templates on the existing code, as computeNRVO was not called on them. And also this covered a regression in a draft patch, that didn't work on function templates.


Repository:
  rC Clang

https://reviews.llvm.org/D47067