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: > ChuanqiXu wrote: > > 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 <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 > > > > 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. > So, I think we are on the same page. > Could you land this patch please? Yeah, I am going to land the patch today and my working pipeline is full now : ) 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