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

[Chuanqi Xu via Phabricator via cfe-commits]

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++, 

[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;

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 
> > > > > > > > 

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

[Chuanqi Xu via Phabricator via cfe-commits]

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
> > > > > > > > > > 

[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:
> 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.
> > > > > > > > > > 
> > > > > > >

[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 
> > > > > > > > > compile

[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;

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 t

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

[Roman Rusyaev via Phabricator via cfe-commits]

rusyaev-roman added a comment.

@ChuanqiXu , the release notes were updated. Could you check and merge please?


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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:
> > > 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 
> > Oh, I forgot you need edit the ReleaseNotes at clang/docs/ReleaseNotes.rst
> I'm going to add a descri

[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;

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 
> 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 slo

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

[Chuanqi Xu via Phabricator via cfe-commits]

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:
> > > 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 
Oh, I forgot you need edit the ReleaseNotes at clang/docs/ReleaseNotes.rst


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

[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;

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 


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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:
> > > 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?


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

[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;

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.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added inline comments.
This revision is now accepted and ready to land.



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:
> > > > 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.


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

[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;

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.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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:
> > 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.


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

[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;

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;
}
```



Comment at: clang/lib/Sema/Scope.cpp:184-185
+  //}
+  if (!getEntity())
+getParent()->NRVO = *NRVO;
 }

ChuanqiXu wrote:
> There is a similar problem. It looks not right if the NRVO of the parent owns 
> a value already.
Yes, this is intention. You can take a look at the above comment.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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;

What if NRVO contains a value already? It is possible due to the value of NRVO 
could be set by its children.



Comment at: clang/lib/Sema/Scope.cpp:184-185
+  //}
+  if (!getEntity())
+getParent()->NRVO = *NRVO;
 }

There is a similar problem. It looks not right if the NRVO of the parent owns a 
value already.


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

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

[Roman Rusyaev via Phabricator via cfe-commits]

rusyaev-roman added a comment.

@ChuanqiXu , I've added additional comments. Could you check again please?


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

[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/include/clang/Sema/Scope.h:213-215
+  /// A single NRVO candidate variable in this scope, or nullptr if the
+  /// candidate is not available/allowed in this scope.
+  Optional NRVO;

ChuanqiXu wrote:
> Now NRVO has three states, None, nullptr and non-null value.But it doesn't 
> show in comments and implementations.
Actually it's used. I'll add additional comments for clarification.



Comment at: clang/lib/Sema/Scope.cpp:151
+void Scope::applyNRVOAndMergeItIntoParent() {
+  if (!NRVO.hasValue())
+// There is no NRVO candidate in the current scope.

ChuanqiXu wrote:
> 
nullptr should be considered below. I'll add a comment to demonstrate why it is.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/include/clang/Sema/Scope.h:213-215
+  /// A single NRVO candidate variable in this scope, or nullptr if the
+  /// candidate is not available/allowed in this scope.
+  Optional NRVO;

Now NRVO has three states, None, nullptr and non-null value.But it doesn't show 
in comments and implementations.



Comment at: clang/lib/Sema/Scope.cpp:151
+void Scope::applyNRVOAndMergeItIntoParent() {
+  if (!NRVO.hasValue())
+// There is no NRVO candidate in the current scope.




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

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

[Roman Rusyaev via Phabricator via cfe-commits]

rusyaev-roman added a comment.

@ChuanqiXu  , could you take a look again? I've updated the original 
implementation.


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

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

[Roman Rusyaev via Phabricator via cfe-commits]

rusyaev-roman added a comment.

In D119792#3658987 , @Izaron wrote:

> Hi!
>
> Unfortunately I don't have time to finish this pull request, so please feel 
> free to take it and get it done =)
>
> (You may reuse the code from this PR or write a completely new implementation)

Ok. I'll continue this work.


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

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

Hi!

Unfortunately I don't have time to finish this pull request, so please feel 
free to take it and get it done =)

(You may reuse the code from this PR or write a completely new implementation)


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/include/clang/Sema/Scope.h:518
 
   void addNRVOCandidate(VarDecl *VD) {
+// every candidate except VD is "spoiled" now, remove them from the set

Izaron wrote:
> ChuanqiXu wrote:
> > Firstly I am wondering why here doesn't check `NRVO.getInt()` to shortcut 
> > directly. But later I found it would change the logic in 
> > `::mergeNRVOIntoParent`:
> > ```
> > void Scope::mergeNRVOIntoParent() {
> >   if (VarDecl *Candidate = NRVO.getPointer()) {
> > if (isDeclScope(Candidate))
> >   Candidate->setNRVOVariable(true);
> >   }
> >   ...
> > ```
> > 
> > It would set NRVO for the candidate in NRVO if it is in current scope. With 
> > the context of `addNRVOCandidate` here, I could judge that the change would 
> > be:
> > ```
> > X test(bool B) {
> >   X x; // before: no nrvo, after: no nrvo (same)
> >   if (B)
> > return x;
> >   X y; // before: no nrvo, after: nrvo (better)
> >   return y; // Now NRVO.getInt()==true and NRVO.getPointer() == y;
> > }
> > ```
> > 
> > Yeah, the behavior is still 'right'. `y` should be NRVO in this case. But 
> > the implementation smell bad, if `NRVO.getInt()` is true, we shouldn't do 
> > any thing. I am not sure if I state my points well. I mean the 
> > implementation might be correct, but it is hard to understand, read and 
> > maintain. It'd better to make the reader avoid doing mathmatics when 
> > reading the codes.
> > I am not sure if I state my points well. I mean the implementation might be 
> > correct, but it is hard to understand, read and maintain. It'd better to 
> > make the reader avoid doing mathmatics when reading the codes.
> I agree that it is really hard to understand and needs to be polished. It 
> took long time for me to construct the code that won't break.
> 
> I think that one of the sources of the complexity is the `NRVO` variable 
> itself.
> If we could change
> ```
> llvm::PointerIntPair NRVO;
> ```
> to something like
> ```
> VarDecl *NRVOCandidate;
> bool InvalidatesParentNRVOCandidates;
> ```
> And maybe rename `setNoNRVO()` to smth like `invalidateNRVOCandidates` and so 
> on.
> 
> What do you think?
From my understanding, 
```
llvm::PointerIntPair NRVO;
```
is structurally equal to
```
VarDecl *NRVOCandidate;
bool InvalidatesParentNRVOCandidates;
```

so it doesn't change anything. My idea is that if it is possible to remove 
`NRVOCandidatesInScope`? Since there would be at most one NRVO candidate in one 
scope. So it should be possible to represent the NRVO candidate by one variable 
if we don't support nested scopes. If it makes sense? Then let me consider the 
case about nested scopes. For the nested scopes, the child scope is responsible 
to inform the parent scope that:
 If any variable in the parent scope is an available NRVO candidate.

So the child should offer a set for unavailable NRVO candidates for the parent 
scope. So we could get the data structure:
```
llvm::SmallMap NRVOCandidatesLattices;
```

`NRVOCandidatesLattices[V] == true` indicates that V is known available in 
current scope and all the child scopes. And `NRVOCandidatesLattices[V] == 
false` indicates that V is known unavailable in current scope and all the child 
scopes.

So we could get the implementation for `addNRVOCandidate(VarDecl *VD)`:
```
if (VD not in NRVOCandidatesLattices) {
NRVOCandidatesLattices[VD] = true; // We met the VD firstly, it should be 
available NRVO candidate.
Update NRVOCandidatesLattices for other variables; // There should be only 
one available variables in NRVOCandidatesLattices.
}
else if (NRVOCandidatesLattices[VD])
nothing;  // We met the available NRVO again, we shouldn't do anything;
else // NRVOCandidatesLattices[VD] == false;
nothing;  // We've already know what happened.
```


And in `mergeNRVOIntoParent `, we could do:
```
VarDecl *VD = try to get the only one available NRVO candidate from 
UnavilableNRVOCandidates;
if (VD && isDeclScope(Candidate))
 set VD as NRVO candidate;

ParentScope->mergeUnavilableNRVOCandidates(UnavilableNRVOCandidates); // We 
could do lattice merges here. The implementation should be trivial.
```

The implementation looks good to me and easy to understand. Maybe we could add 
extra data structure to erase the iterations in `mergeNRVOIntoParent ` and the 
first branch of `addNRVOCandidate `. But they are helpers and wouldn't make the 
framework hard to understand.



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

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added inline comments.



Comment at: clang/include/clang/Sema/Scope.h:518
 
   void addNRVOCandidate(VarDecl *VD) {
+// every candidate except VD is "spoiled" now, remove them from the set

ChuanqiXu wrote:
> Firstly I am wondering why here doesn't check `NRVO.getInt()` to shortcut 
> directly. But later I found it would change the logic in 
> `::mergeNRVOIntoParent`:
> ```
> void Scope::mergeNRVOIntoParent() {
>   if (VarDecl *Candidate = NRVO.getPointer()) {
> if (isDeclScope(Candidate))
>   Candidate->setNRVOVariable(true);
>   }
>   ...
> ```
> 
> It would set NRVO for the candidate in NRVO if it is in current scope. With 
> the context of `addNRVOCandidate` here, I could judge that the change would 
> be:
> ```
> X test(bool B) {
>   X x; // before: no nrvo, after: no nrvo (same)
>   if (B)
> return x;
>   X y; // before: no nrvo, after: nrvo (better)
>   return y; // Now NRVO.getInt()==true and NRVO.getPointer() == y;
> }
> ```
> 
> Yeah, the behavior is still 'right'. `y` should be NRVO in this case. But the 
> implementation smell bad, if `NRVO.getInt()` is true, we shouldn't do any 
> thing. I am not sure if I state my points well. I mean the implementation 
> might be correct, but it is hard to understand, read and maintain. It'd 
> better to make the reader avoid doing mathmatics when reading the codes.
> I am not sure if I state my points well. I mean the implementation might be 
> correct, but it is hard to understand, read and maintain. It'd better to make 
> the reader avoid doing mathmatics when reading the codes.
I agree that it is really hard to understand and needs to be polished. It took 
long time for me to construct the code that won't break.

I think that one of the sources of the complexity is the `NRVO` variable itself.
If we could change
```
llvm::PointerIntPair NRVO;
```
to something like
```
VarDecl *NRVOCandidate;
bool InvalidatesParentNRVOCandidates;
```
And maybe rename `setNoNRVO()` to smth like `invalidateNRVOCandidates` and so 
on.

What do you think?


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/include/clang/Sema/Scope.h:518
 
   void addNRVOCandidate(VarDecl *VD) {
+// every candidate except VD is "spoiled" now, remove them from the set

Firstly I am wondering why here doesn't check `NRVO.getInt()` to shortcut 
directly. But later I found it would change the logic in 
`::mergeNRVOIntoParent`:
```
void Scope::mergeNRVOIntoParent() {
  if (VarDecl *Candidate = NRVO.getPointer()) {
if (isDeclScope(Candidate))
  Candidate->setNRVOVariable(true);
  }
  ...
```

It would set NRVO for the candidate in NRVO if it is in current scope. With the 
context of `addNRVOCandidate` here, I could judge that the change would be:
```
X test(bool B) {
  X x; // before: no nrvo, after: no nrvo (same)
  if (B)
return x;
  X y; // before: no nrvo, after: nrvo (better)
  return y; // Now NRVO.getInt()==true and NRVO.getPointer() == y;
}
```

Yeah, the behavior is still 'right'. `y` should be NRVO in this case. But the 
implementation smell bad, if `NRVO.getInt()` is true, we shouldn't do any 
thing. I am not sure if I state my points well. I mean the implementation might 
be correct, but it is hard to understand, read and maintain. It'd better to 
make the reader avoid doing mathmatics when reading the codes.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

I've looked into the details as much as I can and I don't find explicit errors.

---

It is better to add tests in clang/unittest by using 
`VarDecl::isNRVOVariable()`.




Comment at: clang/include/clang/Sema/Scope.h:533-535
+if (NRVO.getPointer() != nullptr && NRVO.getPointer() != NewNRVO)
+  NRVO.setInt(true);
+NRVO.setPointer(NewNRVO);


The suggested changes smell better.


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

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

Let me explain a bit more =)

The optimizations from this patch are allowed by the Standard (always have been 
allowed). So there is no need to restrict it under a flag or C++ version.

@erichkeane your comment indeed would be applicable if I allowed NRVO for 
non-movable types, that is currently prohibited by the Standard, but allowed in 
the proposal. Luckily my patch doesn't do this.


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

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

In D119792#3389126 , @erichkeane 
wrote:

> So P2025  has not successfully made it 
> through EWG, so this would have to be under a 'flag'.  Also, I believe this 
> will end up being applied to C++23, so it would have to be under a C++23 
> flag, even when we make it a default behavior.

I felt like this particular patch don't really need to wait until the paper 
make it to C++XX.

**RVO** (unnamed return value optimization), a simpler optimization, has been 
used for a very very long time, before they made it mandatory in C++17 
(effectively just describing the status quo in the Standard).

The paper contains an exhaustive set of NRVO use-cases. **13** out of **20** 
cases are already implemented in Clang, and the current patch makes it **17** 
out of **20**. I could send a patch without mentioning the paper at all, but it 
would be harder to track progress.


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

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

So P2025  has not successfully made it through 
EWG, so this would have to be under a 'flag'.  Also, I believe this will end up 
being applied to C++23, so it would have to be under a C++23 flag, even when we 
make it a default behavior.

I don't have a good feeling on the implementation (I've not looked well into 
this stuff), but those two are necessary.


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

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 416116.
Izaron added a comment.
Herald added a project: All.

Rebased the patch on top of D119927 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119792

Files:
  clang/include/clang/Sema/Scope.h
  clang/lib/Sema/Scope.cpp
  clang/test/CodeGenCXX/nrvo.cpp

Index: clang/test/CodeGenCXX/nrvo.cpp
===
--- clang/test/CodeGenCXX/nrvo.cpp
+++ clang/test/CodeGenCXX/nrvo.cpp
@@ -167,81 +167,13 @@
 
 // CHECK-LABEL: @_Z5test3b(
 // CHECK-NEXT:  entry:
-// CHECK-NEXT:[[X:%.*]] = alloca [[CLASS_X:%.*]], align 1
-// CHECK-NEXT:br i1 [[B:%.*]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
-// CHECK:   if.then:
 // CHECK-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT:%.*]]) #[[ATTR5]]
-// CHECK-NEXT:br label [[RETURN:%.*]]
-// CHECK:   if.end:
-// CHECK-NEXT:[[TMP0:%.*]] = getelementptr inbounds [[CLASS_X]], %class.X* [[X]], i32 0, i32 0
-// CHECK-NEXT:call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR5]]
-// CHECK-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]]) #[[ATTR5]]
-// CHECK-NEXT:call void @_ZN1XC1ERKS_(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT]], %class.X* noundef nonnull align 1 dereferenceable(1) [[X]]) #[[ATTR5]]
-// CHECK-NEXT:call void @_ZN1XD1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]]) #[[ATTR5]]
-// CHECK-NEXT:call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR5]]
-// CHECK-NEXT:br label [[RETURN]]
-// CHECK:   return:
 // CHECK-NEXT:ret void
 //
-// CHECK-EH-03-LABEL: @_Z5test3b(
-// CHECK-EH-03-NEXT:  entry:
-// CHECK-EH-03-NEXT:[[X:%.*]] = alloca [[CLASS_X:%.*]], align 1
-// CHECK-EH-03-NEXT:br i1 [[B:%.*]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
-// CHECK-EH-03:   if.then:
-// CHECK-EH-03-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT:%.*]])
-// CHECK-EH-03-NEXT:br label [[RETURN:%.*]]
-// CHECK-EH-03:   if.end:
-// CHECK-EH-03-NEXT:[[TMP0:%.*]] = getelementptr inbounds [[CLASS_X]], %class.X* [[X]], i32 0, i32 0
-// CHECK-EH-03-NEXT:call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR7]]
-// CHECK-EH-03-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-03-NEXT:invoke void @_ZN1XC1ERKS_(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT]], %class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-03-NEXT:to label [[INVOKE_CONT:%.*]] unwind label [[LPAD:%.*]]
-// CHECK-EH-03:   invoke.cont:
-// CHECK-EH-03-NEXT:call void @_ZN1XD1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-03-NEXT:call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR7]]
-// CHECK-EH-03-NEXT:br label [[RETURN]]
-// CHECK-EH-03:   lpad:
-// CHECK-EH-03-NEXT:[[TMP1:%.*]] = landingpad { i8*, i32 }
-// CHECK-EH-03-NEXT:cleanup
-// CHECK-EH-03-NEXT:invoke void @_ZN1XD1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-03-NEXT:to label [[INVOKE_CONT1:%.*]] unwind label [[TERMINATE_LPAD:%.*]]
-// CHECK-EH-03:   invoke.cont1:
-// CHECK-EH-03-NEXT:call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR7]]
-// CHECK-EH-03-NEXT:resume { i8*, i32 } [[TMP1]]
-// CHECK-EH-03:   return:
-// CHECK-EH-03-NEXT:ret void
-// CHECK-EH-03:   terminate.lpad:
-// CHECK-EH-03-NEXT:[[TMP2:%.*]] = landingpad { i8*, i32 }
-// CHECK-EH-03-NEXT:catch i8* null
-// CHECK-EH-03-NEXT:[[TMP3:%.*]] = extractvalue { i8*, i32 } [[TMP2]], 0
-// CHECK-EH-03-NEXT:call void @__clang_call_terminate(i8* [[TMP3]]) #[[ATTR8]]
-// CHECK-EH-03-NEXT:unreachable
-//
-// CHECK-EH-11-LABEL: @_Z5test3b(
-// CHECK-EH-11-NEXT:  entry:
-// CHECK-EH-11-NEXT:[[X:%.*]] = alloca [[CLASS_X:%.*]], align 1
-// CHECK-EH-11-NEXT:br i1 [[B:%.*]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
-// CHECK-EH-11:   if.then:
-// CHECK-EH-11-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT:%.*]])
-// CHECK-EH-11-NEXT:br label [[RETURN:%.*]]
-// CHECK-EH-11:   if.end:
-// CHECK-EH-11-NEXT:[[TMP0:%.*]] = getelementptr inbounds [[CLASS_X]], %class.X* [[X]], i32 0, i32 0
-// CHECK-EH-11-NEXT:call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR7]]
-// CHECK-EH-11-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-11-NEXT:invoke void @_ZN1XC1ERKS_(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT]], %class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

This review will wait for D119927  to be 
merged, as it adds more tests.


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

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

In D119792#3324354 , @Quuxplusone 
wrote:

> I think it would be an extremely good idea to commit all 20 of these test 
> cases to clang/test/CodeGenCXX/, with their current behavior, as a 
> preliminary patch. Then, D119792  can more 
> clearly show (1) what behavior it's changing, (2) what behavior it's keeping 
> the same, and (3) the fact that it's not regressing any behavior.  Also, 
> you'll help future-maintainers by giving them some extra test cases that have 
> already been identified as interesting, even if you personally aren't 
> changing behavior related to those particular test cases.
>
> (I recently took the same tactic with D119772 
>  as a preliminary for D119184 
>  + D119778 
> , and it was very helpful, at least to me. 
> :))

That's a superb idea, thanks! I will soon add all the cases to 
`clang/test/CodeGenCXX/nrvo.cpp` in an isolated patch. It indeed will 
explicitly show improvements in future patches and prevent silent NRVO 
regression.


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

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a reviewer: mizvekov.
Quuxplusone added a comment.

The code is above my pay grade, but FWIW, I super support the intent of this 
patch! Let's get p2025 support into Clang! :)

> The collection of common cases contains 20 examples: §4.1. Examples. Here is 
> the current status of these examples:
>
> [OK] 13 out of 20 examples are working in Clang as expected.
> [FAIL] 13th example: should be considered separately (as part of fixing 
> consteval code)
> [FAIL] 14th example: should be considered separately (as I haven't looked yet 
> how CXXCatchStmt works).
> [FAIL] 18th example: is unfixable now because of Clang's architecture: my 
> comment on the issue.
> [OK with the patch] 7th, 8th, 11th, 15th example: are working with this patch.

I think it would be an extremely good idea to commit all 20 of these test cases 
to clang/test/CodeGenCXX/, with their current behavior, as a preliminary patch. 
Then, D119792  can more clearly show (1) what 
behavior it's changing, (2) what behavior it's keeping the same, and (3) the 
fact that it's not regressing any behavior.  Also, you'll help 
future-maintainers by giving them some extra test cases that have already been 
identified as interesting, even if you personally aren't changing behavior 
related to those particular test cases.

(I recently took the same tactic with D119772 
 as a preliminary for D119184 
 + D119778 
, and it was very helpful, at least to me. :))


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

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 408999.
Izaron added a comment.

Fix Scope::dumpImpl with more precise log


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119792

Files:
  clang/include/clang/Sema/Scope.h
  clang/lib/Sema/Scope.cpp
  clang/test/CodeGenCXX/nrvo.cpp

Index: clang/test/CodeGenCXX/nrvo.cpp
===
--- clang/test/CodeGenCXX/nrvo.cpp
+++ clang/test/CodeGenCXX/nrvo.cpp
@@ -166,88 +166,19 @@
 
 // CHECK-LABEL: @_Z5test3b(
 // CHECK-NEXT:  entry:
-// CHECK-NEXT:[[X:%.*]] = alloca [[CLASS_X:%.*]], align 1
-// CHECK-NEXT:br i1 [[B:%.*]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
-// CHECK:   if.then:
 // CHECK-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT:%.*]]) #[[ATTR5]]
-// CHECK-NEXT:br label [[RETURN:%.*]]
-// CHECK:   if.end:
-// CHECK-NEXT:[[TMP0:%.*]] = getelementptr inbounds [[CLASS_X]], %class.X* [[X]], i32 0, i32 0
-// CHECK-NEXT:call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR5]]
-// CHECK-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]]) #[[ATTR5]]
-// CHECK-NEXT:call void @_ZN1XC1ERKS_(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT]], %class.X* noundef nonnull align 1 dereferenceable(1) [[X]]) #[[ATTR5]]
-// CHECK-NEXT:call void @_ZN1XD1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]]) #[[ATTR5]]
-// CHECK-NEXT:call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR5]]
-// CHECK-NEXT:br label [[RETURN]]
-// CHECK:   return:
 // CHECK-NEXT:ret void
 //
-// CHECK-EH-03-LABEL: @_Z5test3b(
-// CHECK-EH-03-NEXT:  entry:
-// CHECK-EH-03-NEXT:[[X:%.*]] = alloca [[CLASS_X:%.*]], align 1
-// CHECK-EH-03-NEXT:br i1 [[B:%.*]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
-// CHECK-EH-03:   if.then:
-// CHECK-EH-03-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT:%.*]])
-// CHECK-EH-03-NEXT:br label [[RETURN:%.*]]
-// CHECK-EH-03:   if.end:
-// CHECK-EH-03-NEXT:[[TMP0:%.*]] = getelementptr inbounds [[CLASS_X]], %class.X* [[X]], i32 0, i32 0
-// CHECK-EH-03-NEXT:call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR7]]
-// CHECK-EH-03-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-03-NEXT:invoke void @_ZN1XC1ERKS_(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT]], %class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-03-NEXT:to label [[INVOKE_CONT:%.*]] unwind label [[LPAD:%.*]]
-// CHECK-EH-03:   invoke.cont:
-// CHECK-EH-03-NEXT:call void @_ZN1XD1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-03-NEXT:call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR7]]
-// CHECK-EH-03-NEXT:br label [[RETURN]]
-// CHECK-EH-03:   lpad:
-// CHECK-EH-03-NEXT:[[TMP1:%.*]] = landingpad { i8*, i32 }
-// CHECK-EH-03-NEXT:cleanup
-// CHECK-EH-03-NEXT:invoke void @_ZN1XD1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-03-NEXT:to label [[INVOKE_CONT1:%.*]] unwind label [[TERMINATE_LPAD:%.*]]
-// CHECK-EH-03:   invoke.cont1:
-// CHECK-EH-03-NEXT:call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR7]]
-// CHECK-EH-03-NEXT:resume { i8*, i32 } [[TMP1]]
-// CHECK-EH-03:   return:
-// CHECK-EH-03-NEXT:ret void
-// CHECK-EH-03:   terminate.lpad:
-// CHECK-EH-03-NEXT:[[TMP2:%.*]] = landingpad { i8*, i32 }
-// CHECK-EH-03-NEXT:catch i8* null
-// CHECK-EH-03-NEXT:[[TMP3:%.*]] = extractvalue { i8*, i32 } [[TMP2]], 0
-// CHECK-EH-03-NEXT:call void @__clang_call_terminate(i8* [[TMP3]]) #[[ATTR8]]
-// CHECK-EH-03-NEXT:unreachable
-//
-// CHECK-EH-11-LABEL: @_Z5test3b(
-// CHECK-EH-11-NEXT:  entry:
-// CHECK-EH-11-NEXT:[[X:%.*]] = alloca [[CLASS_X:%.*]], align 1
-// CHECK-EH-11-NEXT:br i1 [[B:%.*]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
-// CHECK-EH-11:   if.then:
-// CHECK-EH-11-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT:%.*]])
-// CHECK-EH-11-NEXT:br label [[RETURN:%.*]]
-// CHECK-EH-11:   if.end:
-// CHECK-EH-11-NEXT:[[TMP0:%.*]] = getelementptr inbounds [[CLASS_X]], %class.X* [[X]], i32 0, i32 0
-// CHECK-EH-11-NEXT:call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR7]]
-// CHECK-EH-11-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-11-NEXT:invoke void @_ZN1XC1ERKS_(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT]], %class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-11-NEXT:to label [[INVOKE_CONT:%.*]] unwin

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

Cases that show the difference (they're covered in tests, though do we need an 
AST test as well?):

  X test(bool B) {
if (B) {
  X y; // before: nrvo, after: nrvo (same)
  return y;
}
X x; // before: no nrvo, after: nrvo (better)
return x;
  }

  X test(bool B) {
X x; // before: no nrvo, after: no nrvo (same)
if (B)
  return x;
X y; // before: no nrvo, after: nrvo (better)
return y;
  }

  X test(bool A, bool B) {
{
  {
X x; // before: nrvo, after: nrvo (same)
if (A)
  return x;
  }
  X y; // before: no nrvo, after: nrvo (better)
  if (B)
return y;
}
X z;
retur n z; // before: no nrvo, after: nrvo (better)
  }


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

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

There is a nice proposal (P2025) Guaranteed copy elision for return variables 
 by 
**Anton Zhilin**. The poll on the proposal showed that its ideas are very 
welcome: link to cplusplus/papers github 
. 
Although the proposal is not yet accepted into the Standard in its current 
wording, some ideas are still good and can be implemented "in advance".

> This proposal aims to provide guaranteed copy elision for common cases of 
> local variables being returned from a function, a.k.a. "guaranteed NRVO".

С++ сompiler implementations determine by their own rules whether there will be 
a NRVO (since it is not a required optimization). The proposal contains a 
collection of common cases of local variables being returned where copy elision 
is safe and makes senses and therefore is desirable to do.

The main requirement (omitting details for language-lawyers) is that:

> Copy elision is guaranteed for return x; if every return "seen" by x is 
> return x;

"seen by `x`" means here "all non-discarded return statements in `x`'s 
potential scope". There are more details in the proposal.

The collection of common cases contains 20 examples: §4.1. Examples 
.
Here is the current status of these examples:

- [OK] 13 out of 20 examples are working in Clang as expected.
- [FAIL] 13th example: should be considered separately (as part of fixing 
consteval code)
- [FAIL] 14th example: should be considered separately (as I haven't looked yet 
how `CXXCatchStmt` works).
- [FAIL] 18th example: is unfixable now because of Clang's architecture: my 
comment on the issue 
.
- **[OK with the patch]** 7th, 8th, 11th, 15th example: are working with this 
patch.

In order to make the last group of 4 examples working, there is a need to 
rewrite the logic for calculating the NRVO candidate.
The `clang::Scope` class has methods `void AddDecl(Decl *D)`, `void 
addNRVOCandidate(VarDecl *VD)`, `void setNoNRVO()`, that are called in the 
order of the scope's parsing.

- `void AddDecl(Decl *D)` is called when there is a new `Decl *D` in the scope. 
The `D`'s potential scope starts now.
- `void addNRVOCandidate(VarDecl *VD)` is called when there is a `return 
` in the scope or when a children scope has successfully 
calculated its single NRVO candidate.
- `void setNoNRVO()` is called when there is a `return ` in 
the scope or when a children scope is telling us to drop our NRVO candidates 
(nevertheless, the children scope now can still succesfully calculated the NRVO 
candidate).

We need to have a set of "unspoiled variables" to find the NRVO candidate 
effectively (yeah, introducing some made up terminology...) A variable is 
"unspoiled" if it is not forbidden to be the NRVO candidate yet. An `AddDecl` 
call adds the variable to this set. An `addNRVOCandidate` call "spoils" every 
variable except `VD`. A `setNoNRVO` "spoils" every variable. Only an "unspoiled 
variable" may be the NRVO candidate.


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

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

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron created this revision.
Izaron added reviewers: lebedev.ri, rsmith.
Izaron requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Before the patch we calculated the NRVO candidate looking at the
variable's whole enclosing scope. The research in [P2025 
] shows that looking
at the variable's potential scope is better and covers more cases where NRVO
would be safe and desirable.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119792

Files:
  clang/include/clang/Sema/Scope.h
  clang/test/CodeGenCXX/nrvo.cpp

Index: clang/test/CodeGenCXX/nrvo.cpp
===
--- clang/test/CodeGenCXX/nrvo.cpp
+++ clang/test/CodeGenCXX/nrvo.cpp
@@ -166,88 +166,19 @@
 
 // CHECK-LABEL: @_Z5test3b(
 // CHECK-NEXT:  entry:
-// CHECK-NEXT:[[X:%.*]] = alloca [[CLASS_X:%.*]], align 1
-// CHECK-NEXT:br i1 [[B:%.*]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
-// CHECK:   if.then:
 // CHECK-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT:%.*]]) #[[ATTR5]]
-// CHECK-NEXT:br label [[RETURN:%.*]]
-// CHECK:   if.end:
-// CHECK-NEXT:[[TMP0:%.*]] = getelementptr inbounds [[CLASS_X]], %class.X* [[X]], i32 0, i32 0
-// CHECK-NEXT:call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR5]]
-// CHECK-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]]) #[[ATTR5]]
-// CHECK-NEXT:call void @_ZN1XC1ERKS_(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT]], %class.X* noundef nonnull align 1 dereferenceable(1) [[X]]) #[[ATTR5]]
-// CHECK-NEXT:call void @_ZN1XD1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]]) #[[ATTR5]]
-// CHECK-NEXT:call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR5]]
-// CHECK-NEXT:br label [[RETURN]]
-// CHECK:   return:
 // CHECK-NEXT:ret void
 //
-// CHECK-EH-03-LABEL: @_Z5test3b(
-// CHECK-EH-03-NEXT:  entry:
-// CHECK-EH-03-NEXT:[[X:%.*]] = alloca [[CLASS_X:%.*]], align 1
-// CHECK-EH-03-NEXT:br i1 [[B:%.*]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
-// CHECK-EH-03:   if.then:
-// CHECK-EH-03-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT:%.*]])
-// CHECK-EH-03-NEXT:br label [[RETURN:%.*]]
-// CHECK-EH-03:   if.end:
-// CHECK-EH-03-NEXT:[[TMP0:%.*]] = getelementptr inbounds [[CLASS_X]], %class.X* [[X]], i32 0, i32 0
-// CHECK-EH-03-NEXT:call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR7]]
-// CHECK-EH-03-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-03-NEXT:invoke void @_ZN1XC1ERKS_(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT]], %class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-03-NEXT:to label [[INVOKE_CONT:%.*]] unwind label [[LPAD:%.*]]
-// CHECK-EH-03:   invoke.cont:
-// CHECK-EH-03-NEXT:call void @_ZN1XD1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-03-NEXT:call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR7]]
-// CHECK-EH-03-NEXT:br label [[RETURN]]
-// CHECK-EH-03:   lpad:
-// CHECK-EH-03-NEXT:[[TMP1:%.*]] = landingpad { i8*, i32 }
-// CHECK-EH-03-NEXT:cleanup
-// CHECK-EH-03-NEXT:invoke void @_ZN1XD1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[X]])
-// CHECK-EH-03-NEXT:to label [[INVOKE_CONT1:%.*]] unwind label [[TERMINATE_LPAD:%.*]]
-// CHECK-EH-03:   invoke.cont1:
-// CHECK-EH-03-NEXT:call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR7]]
-// CHECK-EH-03-NEXT:resume { i8*, i32 } [[TMP1]]
-// CHECK-EH-03:   return:
-// CHECK-EH-03-NEXT:ret void
-// CHECK-EH-03:   terminate.lpad:
-// CHECK-EH-03-NEXT:[[TMP2:%.*]] = landingpad { i8*, i32 }
-// CHECK-EH-03-NEXT:catch i8* null
-// CHECK-EH-03-NEXT:[[TMP3:%.*]] = extractvalue { i8*, i32 } [[TMP2]], 0
-// CHECK-EH-03-NEXT:call void @__clang_call_terminate(i8* [[TMP3]]) #[[ATTR8]]
-// CHECK-EH-03-NEXT:unreachable
-//
-// CHECK-EH-11-LABEL: @_Z5test3b(
-// CHECK-EH-11-NEXT:  entry:
-// CHECK-EH-11-NEXT:[[X:%.*]] = alloca [[CLASS_X:%.*]], align 1
-// CHECK-EH-11-NEXT:br i1 [[B:%.*]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
-// CHECK-EH-11:   if.then:
-// CHECK-EH-11-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 dereferenceable(1) [[AGG_RESULT:%.*]])
-// CHECK-EH-11-NEXT:br label [[RETURN:%.*]]
-// CHECK-EH-11:   if.end:
-// CHECK-EH-11-NEXT:[[TMP0:%.*]] = getelementptr inbounds [[CLASS_X]], %class.X* [[X]], i32 0, i32 0
-// CHECK-EH-11-NEXT:call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull [[TMP0]]) #[[ATTR7]]
-// CHECK-EH-11-NEXT:call void @_ZN1XC1Ev(%class.X* noundef nonnull align 1 d