ChuanqiXu 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;
----------------
rusyaev-roman wrote:
> rusyaev-roman 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:
> > > > > > > > > > > > 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 <[email protected]>
> > > > > > 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
> > This is just the first step to support this proposal. All changes in the
> > current patch are allowed by Standard before.
> So, I think the best place for the description of these changes in release
> notes is `C++ Language Changes in Clang` paragraph, because this change is
> improvement and can be done without context of mentioned proposal. What do
> you think?
Got it, your words make sense.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119792/new/
https://reviews.llvm.org/D119792
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
