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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
