[PATCH] D119792: [Clang] [P2025] Analyze only potential scopes for NRVO

[Roman Rusyaev via Phabricator via cfe-commits]

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;
----------------
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.rm at 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119792/new/

https://reviews.llvm.org/D119792