rusyaev-roman added inline comments.
================
Comment at: clang/lib/Sema/Scope.cpp:152-154
+ // Consider the variable as NRVO candidate if the return slot is available
+ // for it in the current scope, or if it can be available in outer scopes.
+ NRVO = CanBePutInReturnSlot ? VD : nullptr;
----------------
ChuanqiXu wrote:
> rusyaev-roman wrote:
> > ChuanqiXu wrote:
> > > rusyaev-roman wrote:
> > > > ChuanqiXu wrote:
> > > > > rusyaev-roman wrote:
> > > > > > ChuanqiXu wrote:
> > > > > > > rusyaev-roman wrote:
> > > > > > > > ChuanqiXu wrote:
> > > > > > > > > rusyaev-roman wrote:
> > > > > > > > > > ChuanqiXu wrote:
> > > > > > > > > > > What if NRVO contains a value already? It is possible due
> > > > > > > > > > > to the value of NRVO could be set by its children.
> > > > > > > > > > Actually this is intention. If the parent has already NRVO
> > > > > > > > > > candidate, then it should be invalidated (or not). Let's
> > > > > > > > > > consider the following examples:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > ```
> > > > > > > > > > X foo(bool b) {
> > > > > > > > > > X x;
> > > > > > > > > > X y;
> > > > > > > > > > if (b)
> > > > > > > > > > return x;
> > > > > > > > > > else
> > > > > > > > > > return y; // when we process this return statement,
> > > > > > > > > > the parent has already NRVO and it will be invalidated
> > > > > > > > > > (this is correct behavior)
> > > > > > > > > > }
> > > > > > > > > > ```
> > > > > > > > > >
> > > > > > > > > > ```
> > > > > > > > > > X foo(bool b) {
> > > > > > > > > > X x;
> > > > > > > > > > if (b)
> > > > > > > > > > return x;
> > > > > > > > > >
> > > > > > > > > > X y;
> > > > > > > > > > // when we process this return statement, the parent has
> > > > > > > > > > already NRVO and it WON't be invalidated
> > > > > > > > > > // (this is correct behavior), because a return slot
> > > > > > > > > > will be available for it
> > > > > > > > > > return y;
> > > > > > > > > > }
> > > > > > > > > > ```
> > > > > > > > > >
> > > > > > > > > > ```
> > > > > > > > > > X foo(bool b) {
> > > > > > > > > > X x;
> > > > > > > > > > if (b)
> > > > > > > > > > return x;
> > > > > > > > > >
> > > > > > > > > > // when we process this return statement, the parent has
> > > > > > > > > > already NRVO and it WON't be invalidated (this is correct
> > > > > > > > > > behavior)
> > > > > > > > > > return x;
> > > > > > > > > > }
> > > > > > > > > > ```
> > > > > > > > > >
> > > > > > > > > > ```
> > > > > > > > > > X foo(bool b, X x) {
> > > > > > > > > > X y;
> > > > > > > > > >
> > > > > > > > > > if (b)
> > > > > > > > > > return x;
> > > > > > > > > >
> > > > > > > > > > // when we process this return statement, the parent
> > > > > > > > > > contains nullptr (invalid candidate) and it will be
> > > > > > > > > > invalidated (this is correct behavior)
> > > > > > > > > > return y;
> > > > > > > > > > }
> > > > > > > > > > ```
> > > > > > > > > >
> > > > > > > > > > ```
> > > > > > > > > > X foo(bool b, X x) {
> > > > > > > > > > if (b)
> > > > > > > > > > return x;
> > > > > > > > > >
> > > > > > > > > > X y;
> > > > > > > > > > // when we process this return statement, the parent
> > > > > > > > > > contains nullptr (invalid candidate) and it WON't be
> > > > > > > > > > invalidated (this is correct behavior)
> > > > > > > > > > return y;
> > > > > > > > > > }
> > > > > > > > > > ```
> > > > > > > > > Oh, I see. Tricky. I don't find invalid cases now. But I
> > > > > > > > > recommend to comment that the children would maintain the
> > > > > > > > > `ReturnSlots` of their parents. (This is anti-intuition)
> > > > > > > > >
> > > > > > > > > Have you tested any larger projects? Like libc++, libstdc++
> > > > > > > > > or something like folly. I feel we need to do such tests to
> > > > > > > > > avoid we get anything wrong.
> > > > > > > > I've already added a comment at the beginning of
> > > > > > > > `updateNRVOCandidate` function where this point is mentioned:
> > > > > > > > ```
> > > > > > > > // ... Therefore, we need to clear return slots for other
> > > > > > > > // variables defined before the current return statement
> > > > > > > > in the current
> > > > > > > > // scope and in outer scopes.
> > > > > > > > ```
> > > > > > > > If it's not enough, please let me know.
> > > > > > > >
> > > > > > > >
> > > > > > > > > Have you tested any larger projects?
> > > > > > > >
> > > > > > > > Yes, I've built the `clang` itself and `compiler-rt` project.
> > > > > > > > Then I've checked them to run 'check-all' (on built clang and
> > > > > > > > compiler-rt). Everything works.
> > > > > > > Great! Clang should be large enough.
> > > > > > Thanks a lot for the careful review!
> > > > > >
> > > > > > @ChuanqiXu , could you land this patch please?
> > > > > >
> > > > > > Many thanks to @Izaron for the original implementation.
> > > > > Sure. What's your prefer Name and Mail address?
> > > > Thanks!
> > > >
> > > > Roman Rusyaev <rusyaev...@gmail.com>
> > > Oh, I forgot you need edit the ReleaseNotes at clang/docs/ReleaseNotes.rst
> > I'm going to add a description in `C++ Language Changes in Clang` paragraph.
> >
> > It will look like:
> > ```
> > - Improved ``copy elision` optimization. It's possible to apply ``NRVO``
> > for an object if at the moment when
> > any return statement of this object is executed, the ``return slot``
> > won't be occupied by another object.
> > ```
> >
> > Is it OK for you?
> According to https://github.com/cplusplus/papers/issues/756, I would like to
> put this in `C++2b Feature Support` section. Although we don't add
> constraints (C++ std >= 23) to do this optimization, this is a C++23 feature
> to C++ standard.
Actually this optimization is just an improvement of existing NRVO optimization
in term of existing standard. This optimization doesn't implement the proposal
itself and can be done without additional flags
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119792/new/
https://reviews.llvm.org/D119792
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
