[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

[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

[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

[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

[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

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

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;

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] D130394: [clang][CodeGen] Factor out Swift ABI hooks (NFCI)

[Roman Rusyaev via Phabricator via cfe-commits]

rusyaev-roman accepted this revision.
rusyaev-roman added a comment.
This revision is now accepted and ready to land.

LGTM. Maybe in the future it's better to use SwiftABIInfo as mix-in like this

  class ARMABIInfo : public ABIInfo, public SwiftABIInfo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130394

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131448: Introduce iterator sentinel to make graph traversal implementation more efficient and cleaner

[Roman Rusyaev via Phabricator via cfe-commits]

rusyaev-roman created this revision.
Herald added subscribers: mtrofin, carlosgalvezp, ormris, wenlei, okura, 
jdoerfert, bmahjour, kuter, arphaman, rogfer01, hiraditya.
Herald added a project: All.
rusyaev-roman requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a reviewer: sstefan1.
Herald added subscribers: cfe-commits, llvm-commits, vkmr.
Herald added projects: LLVM, clang-tools-extra.

As of C++17, for "range-based for loop" the types of the begin-expr and the 
end-expr do not have to be the same,
and in fact the type of the end-expr does not have to be an iterator. This 
makes it possible to delimit
a range by a predicate for graph traversal iterators. To be more specific, it's 
not necessary to comapre
two containers in order to check that an iterator is the end iterator.
For more information see 
https://en.cppreference.com/w/cpp/language/range-for#Notes


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131448

Files:
  clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp
  llvm/include/llvm/ADT/BreadthFirstIterator.h
  llvm/include/llvm/ADT/DepthFirstIterator.h
  llvm/include/llvm/ADT/PostOrderIterator.h
  llvm/include/llvm/ADT/SCCIterator.h
  llvm/include/llvm/ADT/iterator_range.h
  llvm/lib/Analysis/BlockFrequencyInfoImpl.cpp
  llvm/lib/Analysis/BranchProbabilityInfo.cpp
  llvm/lib/Analysis/CFGPrinter.cpp
  llvm/lib/Analysis/DDG.cpp
  llvm/lib/Analysis/DependenceGraphBuilder.cpp
  llvm/lib/Analysis/GlobalsModRef.cpp
  llvm/lib/Analysis/LoopCacheAnalysis.cpp
  llvm/lib/Analysis/LoopNestAnalysis.cpp
  llvm/lib/Analysis/MLInlineAdvisor.cpp
  llvm/lib/Analysis/SyntheticCountsUtils.cpp
  llvm/lib/IR/ModuleSummaryIndex.cpp
  llvm/lib/Transforms/IPO/AttributorAttributes.cpp
  llvm/lib/Transforms/IPO/FunctionAttrs.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
  llvm/lib/Transforms/Utils/FixIrreducible.cpp
  llvm/tools/llvm-profgen/CSPreInliner.cpp
  llvm/tools/opt/PrintSCC.cpp
  llvm/unittests/ADT/DirectedGraphTest.cpp
  llvm/unittests/Transforms/Vectorize/VPlanTest.cpp

Index: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
===
--- llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
+++ llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
@@ -428,7 +428,8 @@
 
 // Post-order.
 FromIterator.clear();
-copy(post_order(Start), std::back_inserter(FromIterator));
+copy(make_range(po_begin(Start), po_end(Start)),
+ std::back_inserter(FromIterator));
 EXPECT_EQ(8u, FromIterator.size());
 EXPECT_EQ(R2BB2, FromIterator[0]);
 EXPECT_EQ(R2BB1, FromIterator[1]);
@@ -630,7 +631,8 @@
 
 // Post-order.
 FromIterator.clear();
-copy(post_order(Start), std::back_inserter(FromIterator));
+copy(make_range(po_begin(Start), po_end(Start)),
+ std::back_inserter(FromIterator));
 EXPECT_EQ(7u, FromIterator.size());
 EXPECT_EQ(VPBB2, FromIterator[0]);
 EXPECT_EQ(R3BB1, FromIterator[1]);
@@ -643,7 +645,8 @@
 // Post-order, const VPRegionBlocks only.
 VPBlockRecursiveTraversalWrapper StartConst(VPBB1);
 SmallVector FromIteratorVPRegion(
-VPBlockUtils::blocksOnly(post_order(StartConst)));
+VPBlockUtils::blocksOnly(
+make_range(po_begin(StartConst), po_end(StartConst;
 EXPECT_EQ(3u, FromIteratorVPRegion.size());
 EXPECT_EQ(R3, FromIteratorVPRegion[0]);
 EXPECT_EQ(R2, FromIteratorVPRegion[1]);
@@ -651,7 +654,8 @@
 
 // Post-order, VPBasicBlocks only.
 FromIterator.clear();
-copy(VPBlockUtils::blocksOnly(post_order(Start)),
+copy(VPBlockUtils::blocksOnly(
+ make_range(po_begin(Start), po_end(Start))),
  std::back_inserter(FromIterator));
 EXPECT_EQ(FromIterator.size(), 4u);
 EXPECT_EQ(VPBB2, FromIterator[0]);
Index: llvm/unittests/ADT/DirectedGraphTest.cpp
===
--- llvm/unittests/ADT/DirectedGraphTest.cpp
+++ llvm/unittests/ADT/DirectedGraphTest.cpp
@@ -270,8 +270,8 @@
   // 2. {N4}
   using NodeListTy = SmallPtrSet;
   SmallVector ListOfSCCs;
-  for (auto &SCC : make_range(scc_begin(&DG), scc_end(&DG)))
-ListOfSCCs.push_back(NodeListTy(SCC.begin(), SCC.end()));
+  for (auto SCC : scc_traversal(&DG))
+ListOfSCCs.push_back(NodeListTy(SCC->begin(), SCC->end()));
 
   EXPECT_TRUE(ListOfSCCs.size() == 2);
 
Index: llvm/tools/opt/PrintSCC.cpp
===
--- llvm/tools/opt/PrintSCC.cpp
+++ llvm/tools/opt/PrintSCC.cpp
@@ -73,14 +73,13 @@
 bool CFGSCC::runOnFunction(Function &F) {
   unsigned sccNum = 0;
   errs() << "SCCs for Function " << F.getName() << " in PostOrder:";
-  for (scc_iterator SCCI = scc_begin(&F); !SCCI.isAtEnd(); ++SCCI) {
-const std::vector &nextSCC = *SCCI;
+  for (auto SCC : scc_traversal(&F)) {
 errs() << "\nSCC #" << ++sccNum << " : ";
-for (BasicBlock 

[PATCH] D131448: Introduce iterator sentinel to make graph traversal implementation more efficient and cleaner

[Roman Rusyaev via Phabricator via cfe-commits]

rusyaev-roman updated this revision to Diff 451208.
rusyaev-roman added a comment.
Herald added subscribers: bzcheeseman, sdasgup3, wenzhicui, wrengr, cota, 
teijeong, rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1, 
Joonsoo, stephenneuendorffer, liufengdb, aartbik, mgester, arpith-jacob, 
nicolasvasilache, antiagainst, shauheen, rriddle, mehdi_amini.
Herald added a reviewer: aartbik.
Herald added a project: MLIR.

Fix mlir build and rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131448

Files:
  clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp
  llvm/include/llvm/ADT/BreadthFirstIterator.h
  llvm/include/llvm/ADT/DepthFirstIterator.h
  llvm/include/llvm/ADT/PostOrderIterator.h
  llvm/include/llvm/ADT/SCCIterator.h
  llvm/include/llvm/ADT/iterator_range.h
  llvm/lib/Analysis/BlockFrequencyInfoImpl.cpp
  llvm/lib/Analysis/BranchProbabilityInfo.cpp
  llvm/lib/Analysis/CFGPrinter.cpp
  llvm/lib/Analysis/DDG.cpp
  llvm/lib/Analysis/DependenceGraphBuilder.cpp
  llvm/lib/Analysis/GlobalsModRef.cpp
  llvm/lib/Analysis/LoopCacheAnalysis.cpp
  llvm/lib/Analysis/LoopNestAnalysis.cpp
  llvm/lib/Analysis/MLInlineAdvisor.cpp
  llvm/lib/Analysis/SyntheticCountsUtils.cpp
  llvm/lib/IR/ModuleSummaryIndex.cpp
  llvm/lib/Transforms/IPO/AttributorAttributes.cpp
  llvm/lib/Transforms/IPO/FunctionAttrs.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
  llvm/lib/Transforms/Utils/FixIrreducible.cpp
  llvm/tools/llvm-profgen/CSPreInliner.cpp
  llvm/tools/opt/PrintSCC.cpp
  llvm/unittests/ADT/DirectedGraphTest.cpp
  llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
  mlir/lib/Analysis/CallGraph.cpp

Index: mlir/lib/Analysis/CallGraph.cpp
===
--- mlir/lib/Analysis/CallGraph.cpp
+++ mlir/lib/Analysis/CallGraph.cpp
@@ -215,9 +215,9 @@
 
   os << "// -- SCCs --\n";
 
-  for (auto &scc : make_range(llvm::scc_begin(this), llvm::scc_end(this))) {
+  for (auto scc : llvm::scc_traversal(this)) {
 os << "// - SCC : \n";
-for (auto &node : scc) {
+for (auto &node : *scc) {
   os << "// -- Node :";
   emitNodeName(node);
   os << "\n";
Index: llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
===
--- llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
+++ llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
@@ -428,7 +428,8 @@
 
 // Post-order.
 FromIterator.clear();
-copy(post_order(Start), std::back_inserter(FromIterator));
+copy(make_range(po_begin(Start), po_end(Start)),
+ std::back_inserter(FromIterator));
 EXPECT_EQ(8u, FromIterator.size());
 EXPECT_EQ(R2BB2, FromIterator[0]);
 EXPECT_EQ(R2BB1, FromIterator[1]);
@@ -630,7 +631,8 @@
 
 // Post-order.
 FromIterator.clear();
-copy(post_order(Start), std::back_inserter(FromIterator));
+copy(make_range(po_begin(Start), po_end(Start)),
+ std::back_inserter(FromIterator));
 EXPECT_EQ(7u, FromIterator.size());
 EXPECT_EQ(VPBB2, FromIterator[0]);
 EXPECT_EQ(R3BB1, FromIterator[1]);
@@ -643,7 +645,8 @@
 // Post-order, const VPRegionBlocks only.
 VPBlockRecursiveTraversalWrapper StartConst(VPBB1);
 SmallVector FromIteratorVPRegion(
-VPBlockUtils::blocksOnly(post_order(StartConst)));
+VPBlockUtils::blocksOnly(
+make_range(po_begin(StartConst), po_end(StartConst;
 EXPECT_EQ(3u, FromIteratorVPRegion.size());
 EXPECT_EQ(R3, FromIteratorVPRegion[0]);
 EXPECT_EQ(R2, FromIteratorVPRegion[1]);
@@ -651,7 +654,8 @@
 
 // Post-order, VPBasicBlocks only.
 FromIterator.clear();
-copy(VPBlockUtils::blocksOnly(post_order(Start)),
+copy(VPBlockUtils::blocksOnly(
+ make_range(po_begin(Start), po_end(Start))),
  std::back_inserter(FromIterator));
 EXPECT_EQ(FromIterator.size(), 4u);
 EXPECT_EQ(VPBB2, FromIterator[0]);
Index: llvm/unittests/ADT/DirectedGraphTest.cpp
===
--- llvm/unittests/ADT/DirectedGraphTest.cpp
+++ llvm/unittests/ADT/DirectedGraphTest.cpp
@@ -270,8 +270,8 @@
   // 2. {N4}
   using NodeListTy = SmallPtrSet;
   SmallVector ListOfSCCs;
-  for (auto &SCC : make_range(scc_begin(&DG), scc_end(&DG)))
-ListOfSCCs.push_back(NodeListTy(SCC.begin(), SCC.end()));
+  for (auto SCC : scc_traversal(&DG))
+ListOfSCCs.push_back(NodeListTy(SCC->begin(), SCC->end()));
 
   EXPECT_TRUE(ListOfSCCs.size() == 2);
 
Index: llvm/tools/opt/PrintSCC.cpp
===
--- llvm/tools/opt/PrintSCC.cpp
+++ llvm/tools/opt/PrintSCC.cpp
@@ -73,14 +73,13 @@
 bool CFGSCC::runOnFunction(Function &F) {
   unsigned sccNum = 0;
   errs() << "SCCs for Function " << F.getName() << " in PostOrder:";
-  for (scc_iterator SCCI = scc_begin(&F); !SC

[PATCH] D131448: Introduce iterator sentinel to make graph traversal implementation more efficient and cleaner

[Roman Rusyaev via Phabricator via cfe-commits]

rusyaev-roman added a comment.

In sum, this change brings the following improvements for all graph traversal 
iterators that are used in 'range-based for loop':

- avoid creating an empty iterator (that holds containers inside) to compare 
with the end iterator
- make 'empty()' call explicit (instead of comparing two containers of 
iterators) when an iterator is compared with the end iterator


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131448

___
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

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

[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/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] D131448: Introduce iterator sentinel to make graph traversal implementation more efficient and cleaner

[Roman Rusyaev via Phabricator via cfe-commits]

rusyaev-roman added a comment.

First of all, thank you for your feedback! I've tried to address all your 
comments.




Comment at: llvm/include/llvm/ADT/BreadthFirstIterator.h:128-131
+  bool operator==(iterator_sentinel) const { return VisitQueue.empty(); }
+
+  bool operator!=(iterator_sentinel RHS) const { return !(*this == RHS); }
+

dblaikie wrote:
> Generally any operator that can be a non-member should be a non-member (but 
> can still be a friend) so there's equal conversion handling for LHS and RHS. 
> Could you make these non-members? (maybe a separate patch to do the same to 
> the existing op overloads, so the new ones don't look weird)
> 
> do you need the inverse operators too, so the sentinel can appear on either 
> side of the comparison? 
Absolutely agree with all your points!

But I didn't want to make the code inconsistent and complicated in this patch. 
So, I suggest making all these operators 'friend' in a separate patch, 
otherwise it can lead to some boilerplate code like this:
```
  friend bool operator==(const scc_iterator &SCCI, iterator_sentinel) {
return SCCI.isAtEnd();
  }

  friend bool operator==(iterator_sentinel IS, const scc_iterator &SCCI) {
return SCCI == IS;
  }

  friend bool operator!=(const scc_iterator &SCCI, iterator_sentinel IS) {
return !(SCCI == IS);
  }

  friend bool operator!=(const scc_iterator &SCCI, iterator_sentinel IS) {
return !(IS == SCCI);
  }
```
This boilerplate code can be avoided using special helper classes, but I 
wouldn't like to implement them in this patch in order to keep it simple.

What do you think?



Comment at: llvm/include/llvm/ADT/SCCIterator.h:49-50
   using SccTy = std::vector;
-  using reference = typename scc_iterator::reference;
+  using reference = const SccTy &;
+  using pointer = const SccTy *;
 

dblaikie wrote:
> does this need a `value` type too? (& then define the `reference` and 
> `pointer` types relative to that)
Thanks, good point! I forgot to add additional types and member functions to 
satisfy `forward iterator` requirements when I removed `iterator_facade` base 
class. I'll update the patch (maybe get iterator_facade back)



Comment at: llvm/include/llvm/ADT/SCCIterator.h:152-162
+bool hasCycle() const {
+  assert(!SCC.empty() && "Dereferencing END SCC iterator!");
+  if (SCC.size() > 1)
+return true;
+  NodeRef N = SCC.front();
+  for (ChildItTy CI = GT::child_begin(N), CE = GT::child_end(N); CI != CE;
+   ++CI)

dblaikie wrote:
> I'm not quite following why this requires the proxy object - even after 
> reading the comment above. It looks like this function is entirely in terms 
> of the `SCC` object that's returned from `operator*` - so maybe this could be 
> a free function, called with `hasCycle(*some_iterator)`?
> maybe this could be a free function, called with hasCycle(*some_iterator)?

This was my initial intention.

But in the case of free function (or maybe static function of scc_iterator 
class) a user should write the following code:
```
 for (const auto& SCC : scc_traversal(Graph))
   if (hasCycle(SCC)) // or in more complicated case when 
GraphTraits cannot be deduced from Graph type -- hasCycle(SCC))
  ...
```

This is the main reason of SCCProxy introduction -- to make it possible to 
write like this:
```
 for (const auto& SCC : scc_traversal(Graph))
   if (SCC.hasCycle())
  ...
```



Comment at: llvm/include/llvm/ADT/SCCIterator.h:165-170
+  SCCProxy operator*() const {
 assert(!CurrentSCC.empty() && "Dereferencing END SCC iterator!");
 return CurrentSCC;
   }
 
+  SCCProxy operator->() const { return operator*(); }

dblaikie wrote:
> I always forget in which cases you're allowed to return a proxy object from 
> an iterator - I thought some iterator concepts (maybe random access is the 
> level at which this kicks in?) that required something that amounts to "there 
> has to be a real object that outlives the iterator"
> 
> Could you refresh my memory on that/on why proxy objects are acceptable for 
> this iterator type? (where/how does this iterator declare what concept it 
> models anyway, since this removed the facade helper?)
A proxy object is allowed to be returned while dereferencing an `input 
iterator` (https://en.cppreference.com/w/cpp/named_req/InputIterator#Notes)

```
The reference type for an input iterator that is not also a 
LegacyForwardIterator does not have to be a reference type: dereferencing an 
input iterator may return a proxy object or value_type itself by value
```

For our case (that's `forward iterator`) we need to satisfy the following thing:
```
 The type std::iterator_traits::reference must be exactly 
   ...
   * const T& otherwise (It is constant), 

(where T is the type denoted by std::iterator_traits::value_type) 
```
I'll also

[PATCH] D131448: Introduce iterator sentinel to make graph traversal implementation more efficient and cleaner

[Roman Rusyaev via Phabricator via cfe-commits]

rusyaev-roman added a comment.

For a while I will be in a place where the Internet does not work well. I'll 
finish the patch when I come back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131448: Introduce iterator sentinel to make graph traversal implementation more efficient and cleaner

[Roman Rusyaev via Phabricator via cfe-commits]

rusyaev-roman added inline comments.



Comment at: llvm/include/llvm/ADT/SCCIterator.h:165-170
+  SCCProxy operator*() const {
 assert(!CurrentSCC.empty() && "Dereferencing END SCC iterator!");
 return CurrentSCC;
   }
 
+  SCCProxy operator->() const { return operator*(); }

dexonsmith wrote:
> dblaikie wrote:
> > rusyaev-roman wrote:
> > > dblaikie wrote:
> > > > I always forget in which cases you're allowed to return a proxy object 
> > > > from an iterator - I thought some iterator concepts (maybe random 
> > > > access is the level at which this kicks in?) that required something 
> > > > that amounts to "there has to be a real object that outlives the 
> > > > iterator"
> > > > 
> > > > Could you refresh my memory on that/on why proxy objects are acceptable 
> > > > for this iterator type? (where/how does this iterator declare what 
> > > > concept it models anyway, since this removed the facade helper?)
> > > A proxy object is allowed to be returned while dereferencing an `input 
> > > iterator` 
> > > (https://en.cppreference.com/w/cpp/named_req/InputIterator#Notes)
> > > 
> > > ```
> > > The reference type for an input iterator that is not also a 
> > > LegacyForwardIterator does not have to be a reference type: dereferencing 
> > > an input iterator may return a proxy object or value_type itself by value
> > > ```
> > > 
> > > For our case (that's `forward iterator`) we need to satisfy the following 
> > > thing:
> > > ```
> > >  The type std::iterator_traits::reference must be exactly 
> > >...
> > >* const T& otherwise (It is constant), 
> > > 
> > > (where T is the type denoted by std::iterator_traits::value_type) 
> > > ```
> > > I'll also update the patch according to this point. Other things are ok 
> > > for using a proxy object.
> > Thanks for doing the legwork/quotations there.
> > 
> > so what's the solution here, if we're going to meet the forward iterator 
> > requirements but want a proxy object?
> IIRC, one solution is to store a proxy object inside the iterator, make 
> `using value_type = ProxyT`, update what the stored proxy points at on 
> `operator*()`, and return `ProxyT&` when dereferencing. But maybe I'm 
> misremembering. (I'm pretty sure `iterator_facade_base` has machinery to help 
> with this stuff, either way; might be worth looking at other uses of it.)
> so what's the solution here, if we're going to meet the forward iterator 
> requirements but want a proxy object?

We need to modify the code according to the above comment. I'm highlighting it 
below.  (maybe, I was not clear. Sorry for this. @dexonsmith has provided more 
detailed explanation)


> For our case (that's forward iterator) we need to satisfy the following thing:
> 
> ```
>  The type std::iterator_traits::reference must be exactly 
>...
>* const T& otherwise (It is constant), 
> 
> (where T is the type denoted by std::iterator_traits::value_type)
> ```
> 
> I'll also update the patch according to this point. Other things are ok for 
> using a proxy object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits