[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Anh Tuyen Tran via Phabricator via cfe-commits]

anhtuyen added a comment.

In D99696#2856370 , @mizvekov wrote:

> @anhtuyen
>
> I pushed a DR with a preliminary fix for it: https://reviews.llvm.org/D105380
>
> This is not ready to merge, still have to add test cases and also decide 
> between a pointed fix like this, or improving the ergonomics of 
> `getDeclAlign` by returning possible failure.
>
> But it does fix your repro.

Thank you very much Matheus @mizvekov for a quick fix.
I have tried your patch, and it worked for my testcase. I will wait for the 
official patch when you commit the changes.
Thanks, again!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

@anhtuyen

I pushed a DR with a preliminary fix for it: https://reviews.llvm.org/D105380

This is not ready to merge, still have to add test cases and also decide 
between a pointed fix like this, or improving the ergonomics of `getDeclAlign` 
by returning possible failure.

But it does fix your repro.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D99696#2856285 , @anhtuyen wrote:

> Hi Matheus @mizvekov,
> The commit 12c90e2e25dfd1d38250055efc21acb42d158912 
>  from 
> this patch seems to cause a regression, where an assertion failure starts to 
> occur with testcases such as

Hi Anh, thanks for reporting this problem!

I confirm it, can reproduce locally.
I will be working on a solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Anh Tuyen Tran via Phabricator via cfe-commits]

anhtuyen added a comment.

Hi Mathew @mizvekov,
The commit 12c90e2e25dfd1d38250055efc21acb42d158912 
 from this 
patch seems to cause a regression, where an assertion failure starts to occur 
with testcases such as

  template  int foo() {
int a alignas(A) = 0;
return a;
  }

To reproduce the issue, please try:

1. Reset the HEAD of your branch to commit 
12c90e2e25dfd1d38250055efc21acb42d158912 
, which 
was just before yours:

  git reset --hard 12c90e2e25dfd1d38250055efc21acb42d158912

2. Build llvm/clamg
3. Compile the above reduced testcase (let's call it test.cpp)

  bin/clang -std=c++11 -c test.cpp

4. There should be no warning nor error

5. Now, please pull in commit 12c90e2e25dfd1d38250055efc21acb42d158912 


  git pull origin 12c90e2e25dfd1d38250055efc21acb42d158912



6. Rebuild and rerun the above compilation command, you will see the following 
assertion failure

  clang: tools/clang/include/clang/AST/AttrImpl.inc:1750: unsigned int 
clang::AlignedAttr::getAlignment(clang::ASTContext &) const: Assertion 
`!isAlignmentDependent()' failed.

GCC does not have that assertion, either.

Can you have a look, please!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG12c90e2e25df: [clang] NRVO: Improvements and handling of 
more cases. (authored by mizvekov).

Changed prior to commit:
  https://reviews.llvm.org/D99696?vs=352566=352586#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/nrvo-tracking.cpp
  clang/test/SemaObjCXX/block-capture.mm

Index: clang/test/SemaObjCXX/block-capture.mm
===
--- /dev/null
+++ clang/test/SemaObjCXX/block-capture.mm
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fobjc-arc -fblocks   -verify=cxx98_20,cxx20%s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fobjc-arc -fblocks   -verify=cxx98_20,cxx98_11 %s
+// RUN: %clang_cc1 -std=c++98 -fsyntax-only -fobjc-arc -fblocks -Wno-c++11-extensions -verify=cxx98_20,cxx98_11 %s
+
+#define TEST(T) void test_##T() { \
+  __block T x;\
+  (void)^(void) { (void)x; }; \
+}
+
+struct CopyOnly {
+  CopyOnly();
+  CopyOnly(CopyOnly &);
+};
+TEST(CopyOnly);
+
+struct CopyNoMove {
+  CopyNoMove();
+  CopyNoMove(CopyNoMove &);
+  CopyNoMove(CopyNoMove &&) = delete; // cxx98_20-note {{marked deleted here}}
+};
+TEST(CopyNoMove); // cxx98_20-error {{call to deleted constructor}}
+
+struct MoveOnly {
+  MoveOnly();
+  MoveOnly(MoveOnly &) = delete;
+  MoveOnly(MoveOnly &&);
+};
+TEST(MoveOnly);
+
+struct NoCopyNoMove {
+  NoCopyNoMove();
+  NoCopyNoMove(NoCopyNoMove &) = delete;
+  NoCopyNoMove(NoCopyNoMove &&) = delete; // cxx98_20-note {{marked deleted here}}
+};
+TEST(NoCopyNoMove); // cxx98_20-error {{call to deleted constructor}}
+
+struct ConvertingRVRef {
+  ConvertingRVRef();
+  ConvertingRVRef(ConvertingRVRef &) = delete; // cxx98_11-note {{marked deleted here}}
+
+  struct X {};
+  ConvertingRVRef(X &&);
+  operator X() const & = delete;
+  operator X() &&;
+};
+TEST(ConvertingRVRef); // cxx98_11-error {{call to deleted constructor}}
+
+struct ConvertingCLVRef {
+  ConvertingCLVRef();
+  ConvertingCLVRef(ConvertingCLVRef &);
+
+  struct X {};
+  ConvertingCLVRef(X &&); // cxx20-note {{passing argument to parameter here}}
+  operator X() const &;
+  operator X() && = delete; // cxx20-note {{marked deleted here}}
+};
+TEST(ConvertingCLVRef); // cxx20-error {{invokes a deleted function}}
+
+struct SubSubMove {};
+struct SubMove : SubSubMove {
+  SubMove();
+  SubMove(SubMove &) = delete; // cxx98_11-note {{marked deleted here}}
+
+  SubMove(SubSubMove &&);
+};
+TEST(SubMove); // cxx98_11-error {{call to deleted constructor}}
Index: clang/test/CodeGen/nrvo-tracking.cpp
===
--- clang/test/CodeGen/nrvo-tracking.cpp
+++ clang/test/CodeGen/nrvo-tracking.cpp
@@ -29,8 +29,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(3, t, T);
@@ -152,7 +150,11 @@
   }; }()();\
 }
 
-//B(1, X); // Uncomment this line at your own peril ;)
+// CHECK-LABEL: define{{.*}} void @_Z2b1v
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+B(1, X);
 
 // CHECK-LABEL: define{{.*}} void @_Z2b2v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
@@ -164,8 +166,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(3, T);
@@ -187,3 +187,26 @@
 B(5, );
 
 #undef B
+
+// CHECK-LABEL: define{{.*}} void @_Z6f_attrv
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+template [[gnu::cdecl]] static inline auto tf_attr() -> X {
+  T t;
+  return t;
+}
+void f_attr() { auto t = tf_attr(); }
+
+// CHECK-LABEL: define{{.*}} void @_Z6b_attrv
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+void b_attr() {
+  auto t = []() {
+return ^X() [[clang::vectorcall]] {
+  T t;
+  return t;
+};
+  }()();
+}
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -23,6 +23,7 @@
 #include 

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov updated this revision to Diff 352566.
mizvekov added a comment.

Fix Arthur's nits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/nrvo-tracking.cpp
  clang/test/SemaObjCXX/block-capture.mm

Index: clang/test/SemaObjCXX/block-capture.mm
===
--- /dev/null
+++ clang/test/SemaObjCXX/block-capture.mm
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fobjc-arc -fblocks   -verify=cxx98_20,cxx20%s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fobjc-arc -fblocks   -verify=cxx98_20,cxx98_11 %s
+// RUN: %clang_cc1 -std=c++98 -fsyntax-only -fobjc-arc -fblocks -Wno-c++11-extensions -verify=cxx98_20,cxx98_11 %s
+
+#define TEST(T) void test_##T() { \
+  __block T x;\
+  (void)^(void) { (void)x; }; \
+}
+
+struct CopyOnly {
+  CopyOnly();
+  CopyOnly(CopyOnly &);
+};
+TEST(CopyOnly);
+
+struct CopyNoMove {
+  CopyNoMove();
+  CopyNoMove(CopyNoMove &);
+  CopyNoMove(CopyNoMove &&) = delete; // cxx98_20-note {{marked deleted here}}
+};
+TEST(CopyNoMove); // cxx98_20-error {{call to deleted constructor}}
+
+struct MoveOnly {
+  MoveOnly();
+  MoveOnly(MoveOnly &) = delete;
+  MoveOnly(MoveOnly &&);
+};
+TEST(MoveOnly);
+
+struct NoCopyNoMove {
+  NoCopyNoMove();
+  NoCopyNoMove(NoCopyNoMove &) = delete;
+  NoCopyNoMove(NoCopyNoMove &&) = delete; // cxx98_20-note {{marked deleted here}}
+};
+TEST(NoCopyNoMove); // cxx98_20-error {{call to deleted constructor}}
+
+struct ConvertingRVRef {
+  ConvertingRVRef();
+  ConvertingRVRef(ConvertingRVRef &) = delete; // cxx98_11-note {{marked deleted here}}
+
+  struct X {};
+  ConvertingRVRef(X &&);
+  operator X() const & = delete;
+  operator X() &&;
+};
+TEST(ConvertingRVRef); // cxx98_11-error {{call to deleted constructor}}
+
+struct ConvertingCLVRef {
+  ConvertingCLVRef();
+  ConvertingCLVRef(ConvertingCLVRef &);
+
+  struct X {};
+  ConvertingCLVRef(X &&); // cxx20-note {{passing argument to parameter here}}
+  operator X() const &;
+  operator X() && = delete; // cxx20-note {{marked deleted here}}
+};
+TEST(ConvertingCLVRef); // cxx20-error {{invokes a deleted function}}
+
+struct SubSubMove {};
+struct SubMove : SubSubMove {
+  SubMove();
+  SubMove(SubMove &) = delete; // cxx98_11-note {{marked deleted here}}
+
+  SubMove(SubSubMove &&);
+};
+TEST(SubMove); // cxx98_11-error {{call to deleted constructor}}
Index: clang/test/CodeGen/nrvo-tracking.cpp
===
--- clang/test/CodeGen/nrvo-tracking.cpp
+++ clang/test/CodeGen/nrvo-tracking.cpp
@@ -29,8 +29,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(3, t, T);
@@ -152,7 +150,11 @@
   }; }()();\
 }
 
-//B(1, X); // Uncomment this line at your own peril ;)
+// CHECK-LABEL: define{{.*}} void @_Z2b1v
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+B(1, X);
 
 // CHECK-LABEL: define{{.*}} void @_Z2b2v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
@@ -164,8 +166,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(3, T);
@@ -187,3 +187,26 @@
 B(5, );
 
 #undef B
+
+// CHECK-LABEL: define{{.*}} void @_Z6f_attrv
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+template [[gnu::cdecl]] static inline auto tf_attr() -> X {
+T t;
+return t;
+}
+void f_attr() { auto t = tf_attr(); }
+
+// CHECK-LABEL: define{{.*}} void @_Z6b_attrv
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+void b_attr() {
+  auto t = []() {
+return ^ X () [[clang::vectorcall]] {
+  T t;
+  return t;
+};
+  }()();
+}
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include 

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

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

Quuxplusone added a comment.

Some tiny nits. I can't think of any interesting block-related cases (but my 
knowledge of blocks is apparently very rusty at this point).




Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1100
+// This is the last chance we have of checking copy elision eligibility
+// for functions in depdendent contexts. The sema actions for building
+// the return statement during template instantiation will have no effect

/depdendent/dependent/



Comment at: clang/test/CodeGen/nrvo-tracking.cpp:206
+void b_attr() {
+  auto t = []() { return ^ X () [[clang::vectorcall]] {
+  T t;

Formatting nit: I'd think
```
auto t = []() {
return ^X() [[clang::vectorcall]] {
T t;
return t;
};
}()();
```
would be less confusing to the eyes.




Comment at: clang/test/SemaObjCXX/block-capture.mm:43
+  ConvertingRVRef(X &&);
+  operator X() const & = delete;;
+  operator X() &&;

Nit: `;;` here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

@rjmccall Added some tests covering these semantics: 
clang/test/SemaObjCXX/block-capture.mm and also some extra documentation to 
checkEscapingByref. Not sure if I got the terminology right here..
@Quuxplusone any other interesting corner cases for these new tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov updated this revision to Diff 352271.
mizvekov added a comment.

-bugs
+tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/nrvo-tracking.cpp
  clang/test/SemaObjCXX/block-capture.mm

Index: clang/test/SemaObjCXX/block-capture.mm
===
--- /dev/null
+++ clang/test/SemaObjCXX/block-capture.mm
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fobjc-arc -fblocks   -verify=cxx98_20,cxx20%s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fobjc-arc -fblocks   -verify=cxx98_20,cxx98_11 %s
+// RUN: %clang_cc1 -std=c++98 -fsyntax-only -fobjc-arc -fblocks -Wno-c++11-extensions -verify=cxx98_20,cxx98_11 %s
+
+#define TEST(T) void test_##T() { \
+  __block T x;\
+  (void)^(void) { (void)x; }; \
+}
+
+struct CopyOnly {
+  CopyOnly();
+  CopyOnly(CopyOnly &);
+};
+TEST(CopyOnly);
+
+struct CopyNoMove {
+  CopyNoMove();
+  CopyNoMove(CopyNoMove &);
+  CopyNoMove(CopyNoMove &&) = delete; // cxx98_20-note {{marked deleted here}}
+};
+TEST(CopyNoMove); // cxx98_20-error {{call to deleted constructor}}
+
+struct MoveOnly {
+  MoveOnly();
+  MoveOnly(MoveOnly &) = delete;
+  MoveOnly(MoveOnly &&);
+};
+TEST(MoveOnly);
+
+struct NoCopyNoMove {
+  NoCopyNoMove();
+  NoCopyNoMove(NoCopyNoMove &) = delete;
+  NoCopyNoMove(NoCopyNoMove &&) = delete; // cxx98_20-note {{marked deleted here}}
+};
+TEST(NoCopyNoMove); // cxx98_20-error {{call to deleted constructor}}
+
+struct ConvertingRVRef {
+  ConvertingRVRef();
+  ConvertingRVRef(ConvertingRVRef &) = delete; // cxx98_11-note {{marked deleted here}}
+
+  struct X {};
+  ConvertingRVRef(X &&);
+  operator X() const & = delete;;
+  operator X() &&;
+};
+TEST(ConvertingRVRef); // cxx98_11-error {{call to deleted constructor}}
+
+struct ConvertingCLVRef {
+  ConvertingCLVRef();
+  ConvertingCLVRef(ConvertingCLVRef &);
+
+  struct X {};
+  ConvertingCLVRef(X &&); // cxx20-note {{passing argument to parameter here}}
+  operator X() const &;
+  operator X() && = delete; // cxx20-note {{marked deleted here}}
+};
+TEST(ConvertingCLVRef); // cxx20-error {{invokes a deleted function}}
+
+struct SubSubMove {};
+struct SubMove : SubSubMove {
+  SubMove();
+  SubMove(SubMove &) = delete; // cxx98_11-note {{marked deleted here}}
+
+  SubMove(SubSubMove &&);
+};
+TEST(SubMove); // cxx98_11-error {{call to deleted constructor}}
Index: clang/test/CodeGen/nrvo-tracking.cpp
===
--- clang/test/CodeGen/nrvo-tracking.cpp
+++ clang/test/CodeGen/nrvo-tracking.cpp
@@ -29,8 +29,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(3, t, T);
@@ -152,7 +150,11 @@
   }; }()();\
 }
 
-//B(1, X); // Uncomment this line at your own peril ;)
+// CHECK-LABEL: define{{.*}} void @_Z2b1v
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+B(1, X);
 
 // CHECK-LABEL: define{{.*}} void @_Z2b2v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
@@ -164,8 +166,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(3, T);
@@ -187,3 +187,24 @@
 B(5, );
 
 #undef B
+
+// CHECK-LABEL: define{{.*}} void @_Z6f_attrv
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+template [[gnu::cdecl]] static inline auto tf_attr() -> X {
+T t;
+return t;
+}
+void f_attr() { auto t = tf_attr(); }
+
+// CHECK-LABEL: define{{.*}} void @_Z6b_attrv
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+void b_attr() {
+  auto t = []() { return ^ X () [[clang::vectorcall]] {
+  T t;
+  return t;
+  }; }()();
+}
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateInstCallback.h"

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D99696#2817126 , @rjmccall wrote:

> When `__block` variables get moved to the heap, they're supposed to be moved 
> if possible, not copied.  It looks like PerformMoveOrCopyInitialization 
> requires NRVO info to be passed in to ever do a move.  Maybe it's being 
> passed in wrong when building `__block` copy expressions in some situation.

Found the culprit, that functionality was implemented in checkEscapingByref, 
and it was a bit of a shocker that such a thing was just hiding in a function 
with such an unassuming name :)
Also surprising to find here another user of the two-step overload resolution, 
that also escaped our radar.
There is a C++ proposal (P2266 ) to replace 
that mechanism with something much simpler.

So since this is an ObjectiveC thing, how is the semantics of this interaction 
defined, just in terms of C++ NRVO?
As this reuses `PerformMoveOrCopyInitialization`, it is also affected by the 
change going into C++20 where the restriction on only converting constructors 
was lifted. Was this accidental and nobody noticed?

For this patch I am going to revert to the original behavior, but I think this 
could be revisited in the future so it gets aligned with the C++2b rules after 
P2266 , which would be just having VarRef be an 
XValue unconditionally and perform the regular copy initialization.

> Was there really not a test case covering those semantics?

Nope...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

When `__block` variables get moved to the heap, they're supposed to be moved if 
possible, not copied.  It looks like PerformMoveOrCopyInitialization requires 
NRVO info to be passed in to ever do a move.  Maybe it's being passed in wrong 
when building `__block` copy expressions in some situation.

Was there really not a test case covering those semantics?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Since it seems more discussion is needed here, I've reverted in 
c60dd3b2626a4d9eefd9f82f9a406b0d28d3fd72 
. Since 
they were hard to tease apart, the revert is for both D99696 
 and D99005 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

> @hans: FYI, that looks related to the immediately following D99005 
> , not D99696 
>  specifically.

No, my test case passes at 0d9e8f5f4b68252c6caa1ef81a30777b2f5d7242 
 but fails 
at 1e50c3d785f4563873ab1ce86559f2a1285b5678 
 (D99696 
), so this does seem to be the one that 
introduced the problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

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

Quuxplusone added a comment.

@hans: FYI, that looks related to the immediately following D99005 
, not D99696  
specifically. I suspect that reverting D99005  
(but leaving D99696  in trunk) would suffice 
to fix that symptom. But I agree I don't see why it should be happening. That 
block code looks reasonable to me at first glance and wasn't intended to be 
affected by D99005  AFAIK.
@mizvekov: See why I'm skeptical about merging big patches back-to-back and/or 
on Fridays? ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Hans Wennborg via Phabricator via cfe-commits]

hans added subscribers: rjmccall, hans.
hans added a comment.

We're seeing new build errors in Chromium after this 
(http://crbug.com/1219457). Here's a reduced example:

  $ cat /tmp/a.mm
  #include 
  
  struct Callback {
// Move-only!
Callback(const Callback&) = delete;
Callback& operator=(const Callback&) = delete;
Callback(Callback&&) = default;
Callback& operator=(Callback&&) = default;
  
void operator()() {}
  };
  
  void f(Callback c) {
__block auto x = std::move(c);
  
(void)^(void) {
  x();
};
  }
  
  $ build.release/bin/clang++ -c /tmp/a.mm -target x86_64-apple-darwin10 
-stdlib=libc++
  /tmp/a.mm:14:16: error: call to deleted constructor of 'Callback'
__block auto x = std::move(c);
 ^
  /tmp/a.mm:5:3: note: 'Callback' has been explicitly marked deleted here
Callback(const Callback&) = delete;
^
  1 error generated.

I don't know enough about blocks to know whether the new error makes sense or 
not.

+rjmccall who knows about blocks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1e50c3d785f4: [clang] NRVO: Improvements and handling of 
more cases. (authored by mizvekov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/nrvo-tracking.cpp

Index: clang/test/CodeGen/nrvo-tracking.cpp
===
--- clang/test/CodeGen/nrvo-tracking.cpp
+++ clang/test/CodeGen/nrvo-tracking.cpp
@@ -29,8 +29,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(3, t, T);
@@ -152,7 +150,11 @@
   }; }()();\
 }
 
-//B(1, X); // Uncomment this line at your own peril ;)
+// CHECK-LABEL: define{{.*}} void @_Z2b1v
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+B(1, X);
 
 // CHECK-LABEL: define{{.*}} void @_Z2b2v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
@@ -164,8 +166,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(3, T);
@@ -187,3 +187,24 @@
 B(5, );
 
 #undef B
+
+// CHECK-LABEL: define{{.*}} void @_Z6f_attrv
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+template [[gnu::cdecl]] static inline auto tf_attr() -> X {
+T t;
+return t;
+}
+void f_attr() { auto t = tf_attr(); }
+
+// CHECK-LABEL: define{{.*}} void @_Z6b_attrv
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+void b_attr() {
+  auto t = []() { return ^ X () [[clang::vectorcall]] {
+  T t;
+  return t;
+  }; }()();
+}
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateInstCallback.h"
@@ -1085,11 +1086,30 @@
 
   SemaRef.BuildVariableInstantiation(Var, D, TemplateArgs, LateAttrs, Owner,
  StartingScope, InstantiatingVarTemplate);
-
   if (D->isNRVOVariable()) {
-QualType ReturnType = cast(DC)->getReturnType();
-if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
-  Var->setNRVOVariable(true);
+QualType RT;
+if (auto *F = dyn_cast(DC))
+  RT = F->getReturnType();
+else if (isa(DC))
+  RT = cast(SemaRef.getCurBlock()->FunctionType)
+   ->getReturnType();
+else
+  llvm_unreachable("Unknown context type");
+
+// This is the last chance we have of checking copy elision eligibility
+// for functions in depdendent contexts. The sema actions for building
+// the return statement during template instantiation will have no effect
+// regarding copy elision, since NRVO propagation runs on the scope exit
+// actions, and these are not run on instantiation.
+// This might run through some VarDecls which were returned from non-taken
+// 'if constexpr' branches, and these will end up being constructed on the
+// return slot even if they will never be returned, as a sort of accidental
+// 'optimization'. Notably, functions with 'auto' return types won't have it
+// deduced by this point. Coupled with the limitation described
+// previously, this makes it very hard to support copy elision for these.
+Sema::NamedReturnInfo Info = SemaRef.getNamedReturnInfo(Var);
+bool NRVO = SemaRef.getCopyElisionCandidate(Info, RT) != nullptr;
+Var->setNRVOVariable(NRVO);
   }
 
   Var->setImplicit(D->isImplicit());
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3307,99 +3307,153 @@
   return new (Context) BreakStmt(BreakLoc);
 }
 
-/// Determine whether the given expression is a candidate for
-/// copy elision in either a return statement or a throw expression.
+/// Determine whether the given expression might be move-eligible or
+/// copy-elidable in either a 

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov updated this revision to Diff 351462.
mizvekov added a comment.

Look through AttributedType when obtaining FunctionDecl return type.

Adds a couple more test cases to cover this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/nrvo-tracking.cpp

Index: clang/test/CodeGen/nrvo-tracking.cpp
===
--- clang/test/CodeGen/nrvo-tracking.cpp
+++ clang/test/CodeGen/nrvo-tracking.cpp
@@ -29,8 +29,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(3, t, T);
@@ -152,7 +150,11 @@
   }; }()();\
 }
 
-//B(1, X); // Uncomment this line at your own peril ;)
+// CHECK-LABEL: define{{.*}} void @_Z2b1v
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+B(1, X);
 
 // CHECK-LABEL: define{{.*}} void @_Z2b2v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
@@ -164,8 +166,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(3, T);
@@ -187,3 +187,24 @@
 B(5, );
 
 #undef B
+
+// CHECK-LABEL: define{{.*}} void @_Z6f_attrv
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+template [[gnu::cdecl]] static inline auto tf_attr() -> X {
+T t;
+return t;
+}
+void f_attr() { auto t = tf_attr(); }
+
+// CHECK-LABEL: define{{.*}} void @_Z6b_attrv
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+void b_attr() {
+  auto t = []() { return ^ X () [[clang::vectorcall]] {
+  T t;
+  return t;
+  }; }()();
+}
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateInstCallback.h"
@@ -1085,11 +1086,30 @@
 
   SemaRef.BuildVariableInstantiation(Var, D, TemplateArgs, LateAttrs, Owner,
  StartingScope, InstantiatingVarTemplate);
-
   if (D->isNRVOVariable()) {
-QualType ReturnType = cast(DC)->getReturnType();
-if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
-  Var->setNRVOVariable(true);
+QualType RT;
+if (auto *F = dyn_cast(DC))
+  RT = F->getReturnType();
+else if (isa(DC))
+  RT = cast(SemaRef.getCurBlock()->FunctionType)
+   ->getReturnType();
+else
+  llvm_unreachable("Unknown context type");
+
+// This is the last chance we have of checking copy elision eligibility
+// for functions in depdendent contexts. The sema actions for building
+// the return statement during template instantiation will have no effect
+// regarding copy elision, since NRVO propagation runs on the scope exit
+// actions, and these are not run on instantiation.
+// This might run through some VarDecls which were returned from non-taken
+// 'if constexpr' branches, and these will end up being constructed on the
+// return slot even if they will never be returned, as a sort of accidental
+// 'optimization'. Notably, functions with 'auto' return types won't have it
+// deduced by this point. Coupled with the limitation described
+// previously, this makes it very hard to support copy elision for these.
+Sema::NamedReturnInfo Info = SemaRef.getNamedReturnInfo(Var);
+bool NRVO = SemaRef.getCopyElisionCandidate(Info, RT) != nullptr;
+Var->setNRVOVariable(NRVO);
   }
 
   Var->setImplicit(D->isImplicit());
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3307,99 +3307,153 @@
   return new (Context) BreakStmt(BreakLoc);
 }
 
-/// Determine whether the given expression is a candidate for
-/// copy elision in either a return statement or a throw expression.
+/// Determine whether the given expression might be move-eligible or
+/// copy-elidable in either a 

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov reopened this revision.
mizvekov added a comment.
This revision is now accepted and ready to land.

Thank you @stella.stamenova and @aeubanks for reporting and reverting this.

Turns out was a silly mistake where we were not digging down through 
AttributedType nodes in order to get the function type.
My bad and will try landing one more time once I do a stage2 build locally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

Crashes on a stage 2 build on Windows:

  ../rel/bin/clang-cl /nologo /showIncludes 
/Foobj/llvm/lib/MC/MCParser/MCParser.MasmParser.obj /c 
../../llvm/lib/MC/MCParser/MasmParser.cpp -D_CRT_SECURE_NO_DEPRECATE 
-D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE 
-D_CRT_NONSTDC_NO_WARNINGS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS 
-D_HAS_EXCEPTIONS=0 -D_UNICODE -DUNICODE -I../../llvm/include 
-Igen/llvm/include /Zi /FS -gline-tables-only -gcodeview-ghash /O2 /Gw 
/Zc:inline /EHs-c- /W4 -Wno-unused-parameter -Wdelete-non-virtual-dtor 
-Wstring-conversion -no-canonical-prefixes -Werror=date-time -fmsc-version=1916 
/Brepro /winsysroot../../../sysroot -Wcovered-switch-default /GR-
  Assertion failed: isa(Val) && "cast() argument of incompatible type!", 
file ../../llvm/include/llvm/Support/Casting.h, line 262
  PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash 
backtrace, preprocessed source, and associated run script.
  Stack dump:
  0.  Program arguments: ../rel/bin/clang-cl /nologo /showIncludes 
/Foobj/llvm/lib/MC/MCParser/MCParser.MasmParser.obj /c 
../../llvm/lib/MC/MCParser/MasmParser.cpp -D_CRT_SECURE_NO_DEPRECATE 
-D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE 
-D_CRT_NONSTDC_NO_WARNINGS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS 
-D_HAS_EXCEPTIONS=0 -D_UNICODE -DUNICODE -I../../llvm/include 
-Igen/llvm/include /Zi /FS -gline-tables-only -gcodeview-ghash /O2 /Gw 
/Zc:inline /EHs-c- /W4 -Wno-unused-parameter -Wdelete-non-virtual-dtor 
-Wstring-conversion -no-canonical-prefixes -Werror=date-time -fmsc-version=1916 
/Brepro /winsysroot../../../sysroot -Wcovered-switch-default /GR-
  1.   parser at end of file
  2.  ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\ostream:320:36: 
instantiating function definition 'std::basic_ostream::operator<<'
  3.  ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocale:503:26: 
instantiating function definition 'std::use_facet>'
  4.  ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocnum:1417:35: 
instantiating function definition 'std::num_put::_Getcat'
  5.  ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocnum:1446:21: 
instantiating function definition 'std::num_put::num_put'
  6.  ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocnum:1504:36: 
instantiating function definition 'std::num_put::do_put'
  7.  ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocale:503:26: 
instantiating function definition 'std::use_facet>'
  8.  ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocnum:163:16: 
instantiating function definition 'std::numpunct::_Getcat'
  9.  ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocnum:157:2: 
instantiating function definition 'std::numpunct::numpunct'
  10. ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocnum:199:7: 
instantiating function definition 'std::numpunct::_Init'
  11. ../../../sysroot\VC\Tools\MSVC\14.16.27023\include\xlocale:662:19: 
instantiating function definition 'std::_Maklocstr'
   #0 0x7ff7a6b48596 HandleAbort 
/b/f/w/llvm-project/build/rel/../../llvm/lib/Support/Windows/Signals.inc:408:0
   #1 0x7ff7a9cee690 raise 
C:\src\llvm-project\build\rel\minkernel\crts\ucrt\src\appcrt\misc\signal.cpp:547:0
   #2 0x7ff7a9ce3fd0 abort 
C:\src\llvm-project\build\rel\minkernel\crts\ucrt\src\appcrt\startup\abort.cpp:71:0
   #3 0x7ff7a9ce4672 common_assert_to_stderr 
C:\src\llvm-project\build\rel\minkernel\crts\ucrt\src\appcrt\startup\assert.cpp:186:0
   #4 0x7ff7a9ce451a _wassert 
C:\src\llvm-project\build\rel\minkernel\crts\ucrt\src\appcrt\startup\assert.cpp:443:0
   #5 0x7ff7a8899933 llvm::PointerUnion::isNull 
/b/f/w/llvm-project/build/rel/../../llvm/include/llvm/ADT/PointerUnion.h:172:0
   #6 0x7ff7a8899933 clang::QualType::isNull 
/b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Type.h:734:0
   #7 0x7ff7a8899933 clang::QualType::getCommonPtr 
/b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Type.h:684:0
   #8 0x7ff7a8899933 clang::QualType::getTypePtr 
/b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Type.h:6428:0
   #9 0x7ff7a8899933 
llvm::simplify_type::getSimplifiedValue 
/b/f/w/llvm-project/build/rel/../../clang/include/clang/AST/Type.h:1318:0
  #10 0x7ff7a8899933 llvm::simplify_type::getSimplifiedValue 
/b/f/w/llvm-project/build/rel/../../llvm/include/llvm/Support/Casting.h:48:0
  #11 0x7ff7a8899933 llvm::isa_impl_wrap::doit 
/b/f/w/llvm-project/build/rel/../../llvm/include/llvm/Support/Casting.h:121:0
  #12 0x7ff7a8899933 llvm::isa 
/b/f/w/llvm-project/build/rel/../../llvm/include/llvm/Support/Casting.h:142:0
  #13 0x7ff7a8899933 llvm::cast 
/b/f/w/llvm-project/build/rel/../../llvm/include/llvm/Support/Casting.h:263:0
  #14 0x7ff7a8899933 clang::TemplateDeclInstantiator::VisitVarDecl(class 
clang::VarDecl *, bool, class llvm::ArrayRef *) 

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Stella Stamenova via Phabricator via cfe-commits]

stella.stamenova added a comment.

It looks like this caused a couple of failures on the Windows LLDB bot due to 
crashes in clang:

https://lab.llvm.org/buildbot/#/builders/83/builds/7142


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG667fbcdd0b2e: [clang] NRVO: Improvements and handling of 
more cases. (authored by mizvekov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/nrvo-tracking.cpp

Index: clang/test/CodeGen/nrvo-tracking.cpp
===
--- clang/test/CodeGen/nrvo-tracking.cpp
+++ clang/test/CodeGen/nrvo-tracking.cpp
@@ -29,8 +29,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(3, t, T);
@@ -152,7 +150,11 @@
   }; }()();\
 }
 
-//B(1, X); // Uncomment this line at your own peril ;)
+// CHECK-LABEL: define{{.*}} void @_Z2b1v
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+B(1, X);
 
 // CHECK-LABEL: define{{.*}} void @_Z2b2v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
@@ -164,8 +166,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(3, T);
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateInstCallback.h"
@@ -1085,11 +1086,30 @@
 
   SemaRef.BuildVariableInstantiation(Var, D, TemplateArgs, LateAttrs, Owner,
  StartingScope, InstantiatingVarTemplate);
-
   if (D->isNRVOVariable()) {
-QualType ReturnType = cast(DC)->getReturnType();
-if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
-  Var->setNRVOVariable(true);
+QualType FT;
+if (auto *F = dyn_cast(DC))
+  FT = F->getType();
+else if (isa(DC))
+  FT = SemaRef.getCurBlock()->FunctionType;
+else
+  llvm_unreachable("Unknown context type");
+
+// This is the last chance we have of checking copy elision eligibility
+// for functions in depdendent contexts. The sema actions for building
+// the return statement during template instantiation will have no effect
+// regarding copy elision, since NRVO propagation runs on the scope exit
+// actions, and these are not run on instantiation.
+// This might run through some VarDecls which were returned from non-taken
+// 'if constexpr' branches, and these will end up being constructed on the
+// return slot even if they will never be returned, as a sort of accidental
+// 'optimization'. Notably, functions with 'auto' return types won't have it
+// deduced by this point. Coupled with the limitation described
+// previously, this makes it very hard to support copy elision for these.
+Sema::NamedReturnInfo Info = SemaRef.getNamedReturnInfo(Var);
+bool NRVO = SemaRef.getCopyElisionCandidate(
+Info, cast(FT)->getReturnType()) != nullptr;
+Var->setNRVOVariable(NRVO);
   }
 
   Var->setImplicit(D->isImplicit());
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3307,99 +3307,153 @@
   return new (Context) BreakStmt(BreakLoc);
 }
 
-/// Determine whether the given expression is a candidate for
-/// copy elision in either a return statement or a throw expression.
+/// Determine whether the given expression might be move-eligible or
+/// copy-elidable in either a (co_)return statement or throw expression,
+/// without considering function return type, if applicable.
 ///
-/// \param ReturnType If we're determining the copy elision candidate for
-/// a return statement, this is the return type of the function. If we're
-/// determining the copy elision candidate for a throw expression, this will
-/// be a NULL type.
+/// \param E The expression being returned from the function or block,
+/// being thrown, or being co_returned from a coroutine.
 ///
-/// \param E The expression being returned from the function or block, or
-/// being thrown.
+/// \param ForceCXX20 Overrides 

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D99696#2808592 , @Quuxplusone wrote:

> @aaronpuchert what's your take on D99696  at 
> this point; is it good to go or still unresolved issues?

Overall this looks sensible to me, and I don't really have anything to add. 
Parts of the code seem a bit complex, but that just seems to reflect the 
complexity of the standard(s).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D99696#2808592 , @Quuxplusone wrote:

> @mizvekov, my understanding is:
>
> - D99696  was temporarily blocked on D103720 
> , but now D103720 
>  is landed and D99696 
>  is unblocked
> - D99696  is a codegen improvement in all 
> language modes, and is also blocking D99005 
> - D99005  is blocking our getting experience 
> with p2266  for C++23
>
> IMHO we should just plow ahead here. @aaronpuchert what's your take on D99696 
>  at this point; is it good to go or still 
> unresolved issues?

No, actually, D99696  (This DR) was not 
blocked on any other DRs, I just put it as parent here for my convenience, 
since I had them all stacked on my workstation to make my life easier.
Sorry if that caused confusion and anyone thought it was blocked

D99005  (The P2266 
 implementation) is blocked just on the present 
DR, but that was because there are common changes and I decided to do them in 
this order since it was not clear when P2266  
impl could be merged.

D103720  was a dependency for D100733 
 proposed by @rsmith, which is a dependency 
of D10  (This is the one that implements 
the 'almost xvalue' thing that Jason Merril was pushing on the reflector, in 
order to replace the two-phase overload resolution).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

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

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

@mizvekov, my understanding is:

- D99696  was temporarily blocked on D103720 
, but now D103720 
 is landed and D99696 
 is unblocked
- D99696  is a codegen improvement in all 
language modes, and is also blocking D99005 
- D99005  is blocking our getting experience 
with p2266  for C++23

IMHO we should just plow ahead here. @aaronpuchert what's your take on D99696 
 at this point; is it good to go or still 
unresolved issues?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov updated this revision to Diff 337903.
mizvekov added a comment.

- Added doc to disallowNRVO
- Also detect implicit return type for blocks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/nrvo-tracking.cpp

Index: clang/test/CodeGen/nrvo-tracking.cpp
===
--- clang/test/CodeGen/nrvo-tracking.cpp
+++ clang/test/CodeGen/nrvo-tracking.cpp
@@ -29,8 +29,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(3, t, T);
@@ -152,7 +150,11 @@
   }; }()();\
 }
 
-//B(1, X); // Uncomment this line at your own peril ;)
+// CHECK-LABEL: define{{.*}} void @_Z2b1v
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+B(1, X);
 
 // CHECK-LABEL: define{{.*}} void @_Z2b2v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
@@ -164,8 +166,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(3, T);
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateInstCallback.h"
@@ -1050,11 +1051,30 @@
 
   SemaRef.BuildVariableInstantiation(Var, D, TemplateArgs, LateAttrs, Owner,
  StartingScope, InstantiatingVarTemplate);
-
   if (D->isNRVOVariable()) {
-QualType ReturnType = cast(DC)->getReturnType();
-if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
-  Var->setNRVOVariable(true);
+QualType FT;
+if (auto *F = dyn_cast(DC))
+  FT = F->getType();
+else if (isa(DC))
+  FT = SemaRef.getCurBlock()->FunctionType;
+else
+  llvm_unreachable("Unknown context type");
+
+// This is the last chance we have of checking copy elision eligibility
+// for functions in depdendent contexts. The sema actions for building
+// the return statement during template instantiation will have no effect
+// regarding copy elision, since NRVO propagation runs on the scope exit
+// actions, and these are not run on instantiation.
+// This might run through some VarDecls which were returned from non-taken
+// 'if constexpr' branches, and these will end up being constructed on the
+// return slot even if they will never be returned, as a sort of accidental
+// 'optimization'. Notably, functions with 'auto' return types won't have it
+// deduced by this point. Coupled with the limitation described
+// previously, this makes it very hard to support copy elision for these.
+Sema::NamedReturnInfo Info = SemaRef.getNamedReturnInfo(Var);
+bool NRVO = SemaRef.getCopyElisionCandidate(
+Info, cast(FT)->getReturnType()) != nullptr;
+Var->setNRVOVariable(NRVO);
   }
 
   Var->setImplicit(D->isImplicit());
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3036,99 +3036,153 @@
   return new (Context) BreakStmt(BreakLoc);
 }
 
-/// Determine whether the given expression is a candidate for
-/// copy elision in either a return statement or a throw expression.
+/// Determine whether the given expression might be move-eligible or
+/// copy-elidable in either a (co_)return statement or throw expression,
+/// without considering function return type, if applicable.
 ///
-/// \param ReturnType If we're determining the copy elision candidate for
-/// a return statement, this is the return type of the function. If we're
-/// determining the copy elision candidate for a throw expression, this will
-/// be a NULL type.
+/// \param E The expression being returned from the function or block,
+/// being thrown, or being co_returned from a coroutine.
 ///
-/// \param E The expression being returned from the function or block, or
-/// being thrown.
+/// \param ForceCXX20 Overrides detection of current language mode

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3066
 
-  if (isCopyElisionCandidate(ReturnType, VD, CESK))
-return VD;
-  return nullptr;
+static void downgradeNRVOResult(Sema::NRVOResult , bool CanMove) {
+  Res.S = std::min(Res.S, CanMove ? Sema::NRVOResult::MoveEligible

Quuxplusone wrote:
> mizvekov wrote:
> > aaronpuchert wrote:
> > > mizvekov wrote:
> > > > mizvekov wrote:
> > > > > Quuxplusone wrote:
> > > > > > mizvekov wrote:
> > > > > > > rsmith wrote:
> > > > > > > > I find this name a little unclear (what do we mean by 
> > > > > > > > "downgrade"?). Can we call this something a bit more specific, 
> > > > > > > > such as `disallowCopyElision` or `disallowNRVO`?
> > > > > > > How about `demoteNRVOStatus`?
> > > > > > I'm with Richard here: if you mean "disallow copy elision," say so!
> > > > > > IMO this should be member functions of `NamedReturnInfo`, and it 
> > > > > > should just be
> > > > > > 
> > > > > > void disallowCopyElision() {
> > > > > > S &= ~CopyElidable;  // obviously this "should" work;
> > > > > > }// ensure that your enum values actually 
> > > > > > make it work
> > > > > > 
> > > > > > void disallowImplicitMove() {
> > > > > > S = None;
> > > > > > }
> > > > > > 
> > > > > > The way you use it below is equivalent to e.g.
> > > > > > 
> > > > > >   if (VD->isExceptionVariable()) {
> > > > > > Info.disallowCopyElision();
> > > > > > if (!hasCXX20)
> > > > > >   Info.disallowImplicitMove();
> > > > > >   }
> > > > > > 
> > > > > > which I think is a lot easier to puzzle out what's going on and how 
> > > > > > it corresponds to the standardese.
> > > > > @Quuxplusone But I did not mean "disallow copy elision", the function 
> > > > > will also disallow implicit move based on a parameter, so would be a 
> > > > > bit misleading.
> > > > > 
> > > > > That solution you proposed though is more repetitive and prone to 
> > > > > error: it does not enforce the invariant that you should always 
> > > > > disallow copy elision when disallowing implicit move.
> > > > > Even if you amend that to having one manipulator function which 
> > > > > enforces the invariant, you still have two bools totaling four 
> > > > > states, where only three states make sense.
> > > > > 
> > > > > I would consider a better name though, not necessarily happy with 
> > > > > this one either.
> > > > Also I did consider making this a method, but this function is more of 
> > > > an implementation detail that is better to hide anyway.
> > > You can have more aptly named functions without dropping the invariant. 
> > > I'd say you only need `disallowCopyElision` though, instead of 
> > > `disallowImplicitMove` you could just directly overwrite with `None`.
> > But the dominant use case here is wanting to choose either one based on the 
> > language mode. So the signature taking a bool is more convenient.
> Just to be clear, I don't see the use-case anywhere as wanting to "choose" 
> anything based on a parameter. As my code indicated, I see the logic here as 
> being simply "For exception variables [or whatever], disallow copy elision. 
> _Additionally_, pre-'20, disallow even implicit move for them." And so on. No 
> functions of boolean parameters, just straightforward English if-thens, based 
> on the English in the standard.
> I get that you're probably not going to write the straightforward code in the 
> end; but I just wanted to push a tiny bit harder to see if anyone else sees 
> what I see. :)
> 
Your argument makes sense to me, but the function is called 5 times with a 
runtime parameter, so it's going to be quite a bit more code.

> just straightforward English if-thens, based on the English in the standard.

Technically aren't we handling different standards at once here? So there is 
actually no language in any standard that contains these particular if-thens.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

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

Quuxplusone added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3066
 
-  if (isCopyElisionCandidate(ReturnType, VD, CESK))
-return VD;
-  return nullptr;
+static void downgradeNRVOResult(Sema::NRVOResult , bool CanMove) {
+  Res.S = std::min(Res.S, CanMove ? Sema::NRVOResult::MoveEligible

mizvekov wrote:
> aaronpuchert wrote:
> > mizvekov wrote:
> > > mizvekov wrote:
> > > > Quuxplusone wrote:
> > > > > mizvekov wrote:
> > > > > > rsmith wrote:
> > > > > > > I find this name a little unclear (what do we mean by 
> > > > > > > "downgrade"?). Can we call this something a bit more specific, 
> > > > > > > such as `disallowCopyElision` or `disallowNRVO`?
> > > > > > How about `demoteNRVOStatus`?
> > > > > I'm with Richard here: if you mean "disallow copy elision," say so!
> > > > > IMO this should be member functions of `NamedReturnInfo`, and it 
> > > > > should just be
> > > > > 
> > > > > void disallowCopyElision() {
> > > > > S &= ~CopyElidable;  // obviously this "should" work;
> > > > > }// ensure that your enum values actually 
> > > > > make it work
> > > > > 
> > > > > void disallowImplicitMove() {
> > > > > S = None;
> > > > > }
> > > > > 
> > > > > The way you use it below is equivalent to e.g.
> > > > > 
> > > > >   if (VD->isExceptionVariable()) {
> > > > > Info.disallowCopyElision();
> > > > > if (!hasCXX20)
> > > > >   Info.disallowImplicitMove();
> > > > >   }
> > > > > 
> > > > > which I think is a lot easier to puzzle out what's going on and how 
> > > > > it corresponds to the standardese.
> > > > @Quuxplusone But I did not mean "disallow copy elision", the function 
> > > > will also disallow implicit move based on a parameter, so would be a 
> > > > bit misleading.
> > > > 
> > > > That solution you proposed though is more repetitive and prone to 
> > > > error: it does not enforce the invariant that you should always 
> > > > disallow copy elision when disallowing implicit move.
> > > > Even if you amend that to having one manipulator function which 
> > > > enforces the invariant, you still have two bools totaling four states, 
> > > > where only three states make sense.
> > > > 
> > > > I would consider a better name though, not necessarily happy with this 
> > > > one either.
> > > Also I did consider making this a method, but this function is more of an 
> > > implementation detail that is better to hide anyway.
> > You can have more aptly named functions without dropping the invariant. I'd 
> > say you only need `disallowCopyElision` though, instead of 
> > `disallowImplicitMove` you could just directly overwrite with `None`.
> But the dominant use case here is wanting to choose either one based on the 
> language mode. So the signature taking a bool is more convenient.
Just to be clear, I don't see the use-case anywhere as wanting to "choose" 
anything based on a parameter. As my code indicated, I see the logic here as 
being simply "For exception variables [or whatever], disallow copy elision. 
_Additionally_, pre-'20, disallow even implicit move for them." And so on. No 
functions of boolean parameters, just straightforward English if-thens, based 
on the English in the standard.
I get that you're probably not going to write the straightforward code in the 
end; but I just wanted to push a tiny bit harder to see if anyone else sees 
what I see. :)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3066
 
-  if (isCopyElisionCandidate(ReturnType, VD, CESK))
-return VD;
-  return nullptr;
+static void downgradeNRVOResult(Sema::NRVOResult , bool CanMove) {
+  Res.S = std::min(Res.S, CanMove ? Sema::NRVOResult::MoveEligible

aaronpuchert wrote:
> mizvekov wrote:
> > mizvekov wrote:
> > > Quuxplusone wrote:
> > > > mizvekov wrote:
> > > > > rsmith wrote:
> > > > > > I find this name a little unclear (what do we mean by 
> > > > > > "downgrade"?). Can we call this something a bit more specific, such 
> > > > > > as `disallowCopyElision` or `disallowNRVO`?
> > > > > How about `demoteNRVOStatus`?
> > > > I'm with Richard here: if you mean "disallow copy elision," say so!
> > > > IMO this should be member functions of `NamedReturnInfo`, and it should 
> > > > just be
> > > > 
> > > > void disallowCopyElision() {
> > > > S &= ~CopyElidable;  // obviously this "should" work;
> > > > }// ensure that your enum values actually make 
> > > > it work
> > > > 
> > > > void disallowImplicitMove() {
> > > > S = None;
> > > > }
> > > > 
> > > > The way you use it below is equivalent to e.g.
> > > > 
> > > >   if (VD->isExceptionVariable()) {
> > > > Info.disallowCopyElision();
> > > > if (!hasCXX20)
> > > >   Info.disallowImplicitMove();
> > > >   }
> > > > 
> > > > which I think is a lot easier to puzzle out what's going on and how it 
> > > > corresponds to the standardese.
> > > @Quuxplusone But I did not mean "disallow copy elision", the function 
> > > will also disallow implicit move based on a parameter, so would be a bit 
> > > misleading.
> > > 
> > > That solution you proposed though is more repetitive and prone to error: 
> > > it does not enforce the invariant that you should always disallow copy 
> > > elision when disallowing implicit move.
> > > Even if you amend that to having one manipulator function which enforces 
> > > the invariant, you still have two bools totaling four states, where only 
> > > three states make sense.
> > > 
> > > I would consider a better name though, not necessarily happy with this 
> > > one either.
> > Also I did consider making this a method, but this function is more of an 
> > implementation detail that is better to hide anyway.
> You can have more aptly named functions without dropping the invariant. I'd 
> say you only need `disallowCopyElision` though, instead of 
> `disallowImplicitMove` you could just directly overwrite with `None`.
But the dominant use case here is wanting to choose either one based on the 
language mode. So the signature taking a bool is more convenient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3144-3145
+/// \param ReturnType This is the return type of the function.
+/// \returns The copy elision candidate, in case the initial return expression
+/// was copy elidable, or nullptr otherwise.
+const VarDecl *Sema::getCopyElisionCandidate(NamedReturnInfo ,

So the return value is trivially deducible from the post-state of `Info` (as 
`Info.isCopyElidable() ? Info.Candidate : nullptr`)? I guess it makes some code 
shorter to write, but generally it's preferable to have fewer implicit 
invariants.

Also it's a getter function that modifies its arguments... not saying there 
isn't precedent in the code, but I'd say less surprising function signatures 
are still better where we can have them.



Comment at: clang/lib/Sema/SemaStmt.cpp:3084-3085
+   bool ForceCXX20) {
+  bool hasCXX11 = getLangOpts().CPlusPlus11 || ForceCXX20,
+   hasCXX20 = getLangOpts().CPlusPlus20 || ForceCXX20;
+  NamedReturnInfo Info{VD, NamedReturnInfo::MoveEligibleAndCopyElidable};

Quuxplusone wrote:
> Nit: please declare variables one-per-line. Change that `,` to a `;` and 
> write `bool` again on line 3085 instead of four space characters.
I don't think the coding standards have a rule about this. I tend to advocate 
the opposite in most cases to avoid repetition, but for `bool` it's probably 
fine to repeat.



Comment at: clang/lib/Sema/SemaStmt.cpp:3066
 
-  if (isCopyElisionCandidate(ReturnType, VD, CESK))
-return VD;
-  return nullptr;
+static void downgradeNRVOResult(Sema::NRVOResult , bool CanMove) {
+  Res.S = std::min(Res.S, CanMove ? Sema::NRVOResult::MoveEligible

mizvekov wrote:
> mizvekov wrote:
> > Quuxplusone wrote:
> > > mizvekov wrote:
> > > > rsmith wrote:
> > > > > I find this name a little unclear (what do we mean by "downgrade"?). 
> > > > > Can we call this something a bit more specific, such as 
> > > > > `disallowCopyElision` or `disallowNRVO`?
> > > > How about `demoteNRVOStatus`?
> > > I'm with Richard here: if you mean "disallow copy elision," say so!
> > > IMO this should be member functions of `NamedReturnInfo`, and it should 
> > > just be
> > > 
> > > void disallowCopyElision() {
> > > S &= ~CopyElidable;  // obviously this "should" work;
> > > }// ensure that your enum values actually make it 
> > > work
> > > 
> > > void disallowImplicitMove() {
> > > S = None;
> > > }
> > > 
> > > The way you use it below is equivalent to e.g.
> > > 
> > >   if (VD->isExceptionVariable()) {
> > > Info.disallowCopyElision();
> > > if (!hasCXX20)
> > >   Info.disallowImplicitMove();
> > >   }
> > > 
> > > which I think is a lot easier to puzzle out what's going on and how it 
> > > corresponds to the standardese.
> > @Quuxplusone But I did not mean "disallow copy elision", the function will 
> > also disallow implicit move based on a parameter, so would be a bit 
> > misleading.
> > 
> > That solution you proposed though is more repetitive and prone to error: it 
> > does not enforce the invariant that you should always disallow copy elision 
> > when disallowing implicit move.
> > Even if you amend that to having one manipulator function which enforces 
> > the invariant, you still have two bools totaling four states, where only 
> > three states make sense.
> > 
> > I would consider a better name though, not necessarily happy with this one 
> > either.
> Also I did consider making this a method, but this function is more of an 
> implementation detail that is better to hide anyway.
You can have more aptly named functions without dropping the invariant. I'd say 
you only need `disallowCopyElision` though, instead of `disallowImplicitMove` 
you could just directly overwrite with `None`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov updated this revision to Diff 337266.
mizvekov added a comment.

- Changed the downgrade* function name to disallowNRVO, given second thought, I 
think it's appropriate name.
- Change to one variable declaration per statement as per Arthur's review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/nrvo-tracking.cpp

Index: clang/test/CodeGen/nrvo-tracking.cpp
===
--- clang/test/CodeGen/nrvo-tracking.cpp
+++ clang/test/CodeGen/nrvo-tracking.cpp
@@ -29,8 +29,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(3, t, T);
@@ -152,7 +150,11 @@
   }; }()();\
 }
 
-//B(1, X); // Uncomment this line at your own peril ;)
+// CHECK-LABEL: define{{.*}} void @_Z2b1v
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+B(1, X);
 
 // CHECK-LABEL: define{{.*}} void @_Z2b2v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
@@ -164,8 +166,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(3, T);
@@ -180,8 +180,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b5v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(5, );
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateInstCallback.h"
@@ -1050,11 +1051,30 @@
 
   SemaRef.BuildVariableInstantiation(Var, D, TemplateArgs, LateAttrs, Owner,
  StartingScope, InstantiatingVarTemplate);
-
   if (D->isNRVOVariable()) {
-QualType ReturnType = cast(DC)->getReturnType();
-if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
-  Var->setNRVOVariable(true);
+QualType FT;
+if (auto *F = dyn_cast(DC))
+  FT = F->getType();
+else if (isa(DC))
+  FT = SemaRef.getCurBlock()->FunctionType;
+else
+  llvm_unreachable("Unknown context type");
+
+// This is the last chance we have of checking copy elision eligibility
+// for functions in depdendent contexts. The sema actions for building
+// the return statement during template instantiation will have no effect
+// regarding copy elision, since NRVO propagation runs on the scope exit
+// actions, and these are not run on instantiation.
+// This might run through some VarDecls which were returned from non-taken
+// 'if constexpr' branches, and these will end up being constructed on the
+// return slot even if they will never be returned, as a sort of accidental
+// 'optimization'. Notably, functions with 'auto' return types won't have it
+// deduced by this point. Coupled with the limitation described
+// previously, this makes it very hard to support copy elision for these.
+Sema::NamedReturnInfo Info = SemaRef.getNamedReturnInfo(Var);
+bool NRVO = SemaRef.getCopyElisionCandidate(
+Info, cast(FT)->getReturnType()) != nullptr;
+Var->setNRVOVariable(NRVO);
   }
 
   Var->setImplicit(D->isImplicit());
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3036,99 +3036,145 @@
   return new (Context) BreakStmt(BreakLoc);
 }
 
-/// Determine whether the given expression is a candidate for
-/// copy elision in either a return statement or a throw expression.
+/// Determine whether the given expression might be move-eligible or
+/// copy-elidable in either a (co_)return statement or throw expression,
+/// without considering function return type, if applicable.
 ///
-/// \param ReturnType If we're determining the copy elision candidate for
-/// a return statement, this is the return type of the function. If we're
-/// 

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

@rsmith:

Actually, there is still something not quite right with this patch that I have 
to finish investigating.

The B5  test should not have gotten copy elision. 
I thought at first that we had gotten the function return type deduced during 
VarDecl instantiation, but we actually have a dependant type instead:

dump of FT:

  FunctionProtoType 0x18597a3ed10 ' (void)' dependent cdecl
  `-BuiltinType 0x185960144e0 '' dependent

I think I assumed wrong that these blocks would behave similarly to lambdas 
with `auto` return.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3066
 
-  if (isCopyElisionCandidate(ReturnType, VD, CESK))
-return VD;
-  return nullptr;
+static void downgradeNRVOResult(Sema::NRVOResult , bool CanMove) {
+  Res.S = std::min(Res.S, CanMove ? Sema::NRVOResult::MoveEligible

mizvekov wrote:
> Quuxplusone wrote:
> > mizvekov wrote:
> > > rsmith wrote:
> > > > I find this name a little unclear (what do we mean by "downgrade"?). 
> > > > Can we call this something a bit more specific, such as 
> > > > `disallowCopyElision` or `disallowNRVO`?
> > > How about `demoteNRVOStatus`?
> > I'm with Richard here: if you mean "disallow copy elision," say so!
> > IMO this should be member functions of `NamedReturnInfo`, and it should 
> > just be
> > 
> > void disallowCopyElision() {
> > S &= ~CopyElidable;  // obviously this "should" work;
> > }// ensure that your enum values actually make it 
> > work
> > 
> > void disallowImplicitMove() {
> > S = None;
> > }
> > 
> > The way you use it below is equivalent to e.g.
> > 
> >   if (VD->isExceptionVariable()) {
> > Info.disallowCopyElision();
> > if (!hasCXX20)
> >   Info.disallowImplicitMove();
> >   }
> > 
> > which I think is a lot easier to puzzle out what's going on and how it 
> > corresponds to the standardese.
> @Quuxplusone But I did not mean "disallow copy elision", the function will 
> also disallow implicit move based on a parameter, so would be a bit 
> misleading.
> 
> That solution you proposed though is more repetitive and prone to error: it 
> does not enforce the invariant that you should always disallow copy elision 
> when disallowing implicit move.
> Even if you amend that to having one manipulator function which enforces the 
> invariant, you still have two bools totaling four states, where only three 
> states make sense.
> 
> I would consider a better name though, not necessarily happy with this one 
> either.
Also I did consider making this a method, but this function is more of an 
implementation detail that is better to hide anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3066
 
-  if (isCopyElisionCandidate(ReturnType, VD, CESK))
-return VD;
-  return nullptr;
+static void downgradeNRVOResult(Sema::NRVOResult , bool CanMove) {
+  Res.S = std::min(Res.S, CanMove ? Sema::NRVOResult::MoveEligible

Quuxplusone wrote:
> mizvekov wrote:
> > rsmith wrote:
> > > I find this name a little unclear (what do we mean by "downgrade"?). Can 
> > > we call this something a bit more specific, such as `disallowCopyElision` 
> > > or `disallowNRVO`?
> > How about `demoteNRVOStatus`?
> I'm with Richard here: if you mean "disallow copy elision," say so!
> IMO this should be member functions of `NamedReturnInfo`, and it should just 
> be
> 
> void disallowCopyElision() {
> S &= ~CopyElidable;  // obviously this "should" work;
> }// ensure that your enum values actually make it work
> 
> void disallowImplicitMove() {
> S = None;
> }
> 
> The way you use it below is equivalent to e.g.
> 
>   if (VD->isExceptionVariable()) {
> Info.disallowCopyElision();
> if (!hasCXX20)
>   Info.disallowImplicitMove();
>   }
> 
> which I think is a lot easier to puzzle out what's going on and how it 
> corresponds to the standardese.
@Quuxplusone But I did not mean "disallow copy elision", the function will also 
disallow implicit move based on a parameter, so would be a bit misleading.

That solution you proposed though is more repetitive and prone to error: it 
does not enforce the invariant that you should always disallow copy elision 
when disallowing implicit move.
Even if you amend that to having one manipulator function which enforces the 
invariant, you still have two bools totaling four states, where only three 
states make sense.

I would consider a better name though, not necessarily happy with this one 
either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

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

Quuxplusone added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3084-3085
+   bool ForceCXX20) {
+  bool hasCXX11 = getLangOpts().CPlusPlus11 || ForceCXX20,
+   hasCXX20 = getLangOpts().CPlusPlus20 || ForceCXX20;
+  NamedReturnInfo Info{VD, NamedReturnInfo::MoveEligibleAndCopyElidable};

Nit: please declare variables one-per-line. Change that `,` to a `;` and write 
`bool` again on line 3085 instead of four space characters.



Comment at: clang/lib/Sema/SemaStmt.cpp:3066
 
-  if (isCopyElisionCandidate(ReturnType, VD, CESK))
-return VD;
-  return nullptr;
+static void downgradeNRVOResult(Sema::NRVOResult , bool CanMove) {
+  Res.S = std::min(Res.S, CanMove ? Sema::NRVOResult::MoveEligible

mizvekov wrote:
> rsmith wrote:
> > I find this name a little unclear (what do we mean by "downgrade"?). Can we 
> > call this something a bit more specific, such as `disallowCopyElision` or 
> > `disallowNRVO`?
> How about `demoteNRVOStatus`?
I'm with Richard here: if you mean "disallow copy elision," say so!
IMO this should be member functions of `NamedReturnInfo`, and it should just be

void disallowCopyElision() {
S &= ~CopyElidable;  // obviously this "should" work;
}// ensure that your enum values actually make it work

void disallowImplicitMove() {
S = None;
}

The way you use it below is equivalent to e.g.

  if (VD->isExceptionVariable()) {
Info.disallowCopyElision();
if (!hasCXX20)
  Info.disallowImplicitMove();
  }

which I think is a lot easier to puzzle out what's going on and how it 
corresponds to the standardese.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3290
 }
-
-if (!getLangOpts().CPlusPlus20 && NeedSecondOverloadResolution &&
-!getDiagnostics().isIgnored(diag::warn_return_std_move,
+if (!getDiagnostics().isIgnored(diag::warn_return_std_move,
 Value->getExprLoc())) {

mizvekov wrote:
> mizvekov wrote:
> > rsmith wrote:
> > > Should we skip this if `NRVORes.isMoveEligible() && 
> > > getLangOpts().CPlusPlus20`? In that case, we already tried an 
> > > unrestricted move a couple of lines ago, so we know that adding 
> > > `std::move` won't help. (I think we can still hit the "adding `std::move` 
> > > will help" case in C++20, but only in obscure corners such as a 
> > > `volatile`-qualified local variable.)
> > This goes to the documentation on `getNamedReturnInfo`,  return value, 
> > where it says that the Canidate member will be non-null in case implicit 
> > move would be permitted under the most permissive language standard.
> > 
> > So if Try
> > 
> > So if we are guarded under that condition, we would never 
> Skipping it is just a small compilation performance optimization:
> 
> We are going to try that `TryMoveInitialization` again just below, this time 
> with equivalent of `ForceCXX20 = true`. If we were already in c++20 mode, 
> then that second call is going to be made with basically the same parameters 
> and perform basically the same work and reach the same conclusion: Faliure, 
> and the diagnostic won't be produced.
> 
> Now my take on why that looks confusing: To me `TryMoveInitialization` has a 
> similar problem to the one I addressed in this patch for 
> `getCopyElisionCandidate`, it relies on you calling it multiple times, once 
> to try the the actual work, and if that fails, then you have to call it 
> again, this time in 'permissive' more, to get the information you need for 
> the diagnostics.
> 
> Now I did not try giving it the same treatment of returning an aggregate with 
> all the information you need, so you can call it just once with one less 
> parameter.
> If you agree on that take, I'd be happy to try a similar improvement there as 
> well.
> If you don't think it's worth the noise, then it's all good, no worries :)
> 
> As a side note, I just noticed that the
> `if (QT->isLValueReferenceType()) {` below is dead code, we are never going 
> to consider this move eligible in any mode, and so Res.Candidate is going to 
> be null.
> 
> We should not need to repeat here the checks we already do in getNRVOResult, 
> so I am going to go ahead and remove it.
> 
> I think it may have been added because until just recently 
> `getCopyElisionCandidate` was bugged and accepted LValue references, and I 
> guess the problem was "fixed" in this diagnostic first.
Disregard this comment, I realized I was answering the wrong question and for 
some reason it was not discarded.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:4728
 
-  enum CopyElisionSemanticsKind {
-CES_Strict = 0,
-CES_AllowParameters = 1,
-CES_AllowDifferentTypes = 2,
-CES_AllowExceptionVariables = 4,
-CES_AllowRValueReferenceType = 8,
-CES_ImplicitlyMovableCXX11CXX14CXX17 =
-(CES_AllowParameters | CES_AllowDifferentTypes),
-CES_ImplicitlyMovableCXX20 =
-(CES_AllowParameters | CES_AllowDifferentTypes |
- CES_AllowExceptionVariables | CES_AllowRValueReferenceType),
+  struct NRVOResult {
+const VarDecl *Candidate;

rsmith wrote:
> We generally use `FooResult` to mean "either a `Foo` or an indicator that an 
> error has been diagnosed". Would the name `NRVOInfo` work instead?
> 
> Actually, I think the "O" here is misleading, because we're not talking about 
> performing the optimization here, just about what variable was named in a 
> return statement. And I think calling this "NRVO" is a bit misleading because 
> it also covers implicit move, which isn't traditionally part of NRVO. So how 
> about `NamedReturnValueInfo` or `NamedReturnInfo` or `NamedReturnValue` or 
> `ReturnValueName` or something like that?
I like `NamedReturnInfo`, will change.



Comment at: clang/lib/Sema/SemaStmt.cpp:3066
 
-  if (isCopyElisionCandidate(ReturnType, VD, CESK))
-return VD;
-  return nullptr;
+static void downgradeNRVOResult(Sema::NRVOResult , bool CanMove) {
+  Res.S = std::min(Res.S, CanMove ? Sema::NRVOResult::MoveEligible

rsmith wrote:
> I find this name a little unclear (what do we mean by "downgrade"?). Can we 
> call this something a bit more specific, such as `disallowCopyElision` or 
> `disallowNRVO`?
How about `demoteNRVOStatus`?



Comment at: clang/lib/Sema/SemaStmt.cpp:3156
+if (AT->isCanonicalUnqualified())
+  return Res = NRVOResult(), nullptr;
+

rsmith wrote:
> Please add braces rather than using a comma expression here and below.
Using short utility lambda as an alternative to keep the main logic clear from 
clutter.



Comment at: clang/lib/Sema/SemaStmt.cpp:3290
 }
-
-if (!getLangOpts().CPlusPlus20 && NeedSecondOverloadResolution &&
-!getDiagnostics().isIgnored(diag::warn_return_std_move,
+if (!getDiagnostics().isIgnored(diag::warn_return_std_move,
 Value->getExprLoc())) {

rsmith wrote:
> Should we skip this if `NRVORes.isMoveEligible() && 
> getLangOpts().CPlusPlus20`? In that case, we already tried an unrestricted 
> move a couple of lines ago, so we know that adding `std::move` won't help. (I 
> think we can still hit the "adding `std::move` will help" case in C++20, but 
> only in obscure corners such as a `volatile`-qualified local variable.)
This goes to the documentation on `getNamedReturnInfo`,  return value, where it 
says that the Canidate member will be non-null in case implicit move would be 
permitted under the most permissive language standard.

So if Try

So if we are guarded under that condition, we would never 



Comment at: clang/lib/Sema/SemaStmt.cpp:3290
 }
-
-if (!getLangOpts().CPlusPlus20 && NeedSecondOverloadResolution &&
-!getDiagnostics().isIgnored(diag::warn_return_std_move,
+if (!getDiagnostics().isIgnored(diag::warn_return_std_move,
 Value->getExprLoc())) {

mizvekov wrote:
> rsmith wrote:
> > Should we skip this if `NRVORes.isMoveEligible() && 
> > getLangOpts().CPlusPlus20`? In that case, we already tried an unrestricted 
> > move a couple of lines ago, so we know that adding `std::move` won't help. 
> > (I think we can still hit the "adding `std::move` will help" case in C++20, 
> > but only in obscure corners such as a `volatile`-qualified local variable.)
> This goes to the documentation on `getNamedReturnInfo`,  return value, where 
> it says that the Canidate member will be non-null in case implicit move would 
> be permitted under the most permissive language standard.
> 
> So if Try
> 
> So if we are guarded under that condition, we would never 
Skipping it is just a small compilation performance optimization:

We are going to try that `TryMoveInitialization` again just below, this time 
with equivalent of `ForceCXX20 = true`. If we were already in c++20 mode, then 
that second call is going to be made with basically the same parameters and 
perform basically the same work and reach the same conclusion: Faliure, and the 
diagnostic won't be produced.

Now my take on why that looks confusing: To me `TryMoveInitialization` has a 
similar problem to the one I addressed in this patch for 
`getCopyElisionCandidate`, it relies on you calling it multiple times, once to 
try the the actual work, and if that fails, then you have to call it again, 
this time in 'permissive' more, to get the information you 

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov updated this revision to Diff 337248.
mizvekov marked 7 inline comments as done.
mizvekov added a comment.

- Address rsmith review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/nrvo-tracking.cpp

Index: clang/test/CodeGen/nrvo-tracking.cpp
===
--- clang/test/CodeGen/nrvo-tracking.cpp
+++ clang/test/CodeGen/nrvo-tracking.cpp
@@ -29,8 +29,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(3, t, T);
@@ -152,7 +150,11 @@
   }; }()();\
 }
 
-//B(1, X); // Uncomment this line at your own peril ;)
+// CHECK-LABEL: define{{.*}} void @_Z2b1v
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+B(1, X);
 
 // CHECK-LABEL: define{{.*}} void @_Z2b2v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
@@ -164,8 +166,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(3, T);
@@ -180,8 +180,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b5v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(5, );
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -23,6 +23,7 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/Initialization.h"
 #include "clang/Sema/Lookup.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateInstCallback.h"
@@ -1050,11 +1051,30 @@
 
   SemaRef.BuildVariableInstantiation(Var, D, TemplateArgs, LateAttrs, Owner,
  StartingScope, InstantiatingVarTemplate);
-
   if (D->isNRVOVariable()) {
-QualType ReturnType = cast(DC)->getReturnType();
-if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
-  Var->setNRVOVariable(true);
+QualType FT;
+if (auto *F = dyn_cast(DC))
+  FT = F->getType();
+else if (isa(DC))
+  FT = SemaRef.getCurBlock()->FunctionType;
+else
+  llvm_unreachable("Unknown context type");
+
+// This is the last chance we have of checking copy elision eligibility
+// for functions in depdendent contexts. The sema actions for building
+// the return statement during template instantiation will have no effect
+// regarding copy elision, since NRVO propagation runs on the scope exit
+// actions, and these are not run on instantiation.
+// This might run through some VarDecls which were returned from non-taken
+// 'if constexpr' branches, and these will end up being constructed on the
+// return slot even if they will never be returned, as a sort of accidental
+// 'optimization'. Notably, functions with 'auto' return types won't have it
+// deduced by this point. Coupled with the limitation described
+// previously, this makes it very hard to support copy elision for these.
+Sema::NamedReturnInfo Info = SemaRef.getNamedReturnInfo(Var);
+bool NRVO = SemaRef.getCopyElisionCandidate(
+Info, cast(FT)->getReturnType()) != nullptr;
+Var->setNRVOVariable(NRVO);
   }
 
   Var->setImplicit(D->isImplicit());
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3036,99 +3036,145 @@
   return new (Context) BreakStmt(BreakLoc);
 }
 
-/// Determine whether the given expression is a candidate for
-/// copy elision in either a return statement or a throw expression.
+/// Determine whether the given expression might be move-eligible or
+/// copy-elidable in either a (co_)return statement or throw expression,
+/// without considering function return type, if applicable.
 ///
-/// \param ReturnType If we're determining the copy elision candidate for
-/// a return statement, this is the return type of the function. If we're
-/// determining the copy elision candidate for a throw expression, this will
-/// be a NULL type.
+/// \param E The 

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3150-3153
+  // If we got a non-deduced auto ReturnType, we are in a dependent context and
+  // there is no point in allowing copy elision since we won't have it deduced
+  // by the point the VardDecl is instantiated, which is the last chance we 
have
+  // of deciding if the candidate is really copy elisible.

mizvekov wrote:
> mizvekov wrote:
> > rsmith wrote:
> > > How does this happen? Are there any cases where we could do NRVO or 
> > > should do an implicit move that are blocked by this?
> > > 
> > > It seems to me that we should (nearly always) be able to work out whether 
> > > copy elision is possible here without knowing the deduced type:
> > > -- if the return type is //cv// `auto` then it will always be deduced to 
> > > the type of the returned variable, so we can always perform copy elision
> > > -- if the return type is `decltype(auto)`, then we can perform copy 
> > > elision if the expression is unparenthesized and otherwise cannot; we 
> > > could perhaps track whether the expression was parenthesized in 
> > > `NRVOResult`, and can conservatively disallow copy elision if we don't 
> > > know (eg, from template instantiation, where we're only looking at the 
> > > variable and not the return statements)
> > > -- if the return type is anything else involving `auto`, it can't 
> > > possibly instantiate to a class type, so we'll never perform copy elision
> > Yeah, what you suggested is what I tried on a previous patch in this DR, 
> > but then studying the NRVO tracker carefully I thought about this counter 
> > example:
> > ```
> > template static auto bar() {
> >   { 
> > Foo foo;
> > if constexpr(B) 
> >   return foo;  
> >   } 
> >   { 
> > Bar bar;
> > if constexpr(!B)
> >   return bar;   
> >   } 
> > }   
> > 
> > 
> > Since we run the tracker before instantiation, we would see both return 
> > statements and mark both foo and bar as NRVO variables.
> > Ofcourse in the B = false case, we would end up constructing a Foo in a Bar 
> > return slot
> > 
> > As a side note, It is actually funny that we currently perform this 
> > optimization (most likely accidentally):
> > ```
> > template static Foo bar() {
> >   { 
> > Foo foo1;
> > if constexpr(B) 
> >   return foo1;  
> >   } 
> >   {
> > Foo foo2;
> > return foo2 
> >   } 
> > }
> > ```
> > In the B = false case, we end up constructing foo1 in the return slot even 
> > though we actually never return it.
> Compiler explorer link for the accidental optimization I am talking about: 
> https://godbolt.org/z/czdWodW87
That's amazing, and makes total sense. Yeah, `if constexpr` pretty thoroughly 
destroys the assumptions we make in the `Scope`-based NRVO computation about 
what a templated function will look like after instantiation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3150-3153
+  // If we got a non-deduced auto ReturnType, we are in a dependent context and
+  // there is no point in allowing copy elision since we won't have it deduced
+  // by the point the VardDecl is instantiated, which is the last chance we 
have
+  // of deciding if the candidate is really copy elisible.

mizvekov wrote:
> rsmith wrote:
> > How does this happen? Are there any cases where we could do NRVO or should 
> > do an implicit move that are blocked by this?
> > 
> > It seems to me that we should (nearly always) be able to work out whether 
> > copy elision is possible here without knowing the deduced type:
> > -- if the return type is //cv// `auto` then it will always be deduced to 
> > the type of the returned variable, so we can always perform copy elision
> > -- if the return type is `decltype(auto)`, then we can perform copy elision 
> > if the expression is unparenthesized and otherwise cannot; we could perhaps 
> > track whether the expression was parenthesized in `NRVOResult`, and can 
> > conservatively disallow copy elision if we don't know (eg, from template 
> > instantiation, where we're only looking at the variable and not the return 
> > statements)
> > -- if the return type is anything else involving `auto`, it can't possibly 
> > instantiate to a class type, so we'll never perform copy elision
> Yeah, what you suggested is what I tried on a previous patch in this DR, but 
> then studying the NRVO tracker carefully I thought about this counter example:
> ```
> template static auto bar() {
>   { 
> Foo foo;
> if constexpr(B) 
>   return foo;  
>   } 
>   { 
> Bar bar;
> if constexpr(!B)
>   return bar;   
>   } 
> }   
> 
> 
> Since we run the tracker before instantiation, we would see both return 
> statements and mark both foo and bar as NRVO variables.
> Ofcourse in the B = false case, we would end up constructing a Foo in a Bar 
> return slot
> 
> As a side note, It is actually funny that we currently perform this 
> optimization (most likely accidentally):
> ```
> template static Foo bar() {
>   { 
> Foo foo1;
> if constexpr(B) 
>   return foo1;  
>   } 
>   {
> Foo foo2;
> return foo2 
>   } 
> }
> ```
> In the B = false case, we end up constructing foo1 in the return slot even 
> though we actually never return it.
Compiler explorer link for the accidental optimization I am talking about: 
https://godbolt.org/z/czdWodW87


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3150-3153
+  // If we got a non-deduced auto ReturnType, we are in a dependent context and
+  // there is no point in allowing copy elision since we won't have it deduced
+  // by the point the VardDecl is instantiated, which is the last chance we 
have
+  // of deciding if the candidate is really copy elisible.

rsmith wrote:
> How does this happen? Are there any cases where we could do NRVO or should do 
> an implicit move that are blocked by this?
> 
> It seems to me that we should (nearly always) be able to work out whether 
> copy elision is possible here without knowing the deduced type:
> -- if the return type is //cv// `auto` then it will always be deduced to the 
> type of the returned variable, so we can always perform copy elision
> -- if the return type is `decltype(auto)`, then we can perform copy elision 
> if the expression is unparenthesized and otherwise cannot; we could perhaps 
> track whether the expression was parenthesized in `NRVOResult`, and can 
> conservatively disallow copy elision if we don't know (eg, from template 
> instantiation, where we're only looking at the variable and not the return 
> statements)
> -- if the return type is anything else involving `auto`, it can't possibly 
> instantiate to a class type, so we'll never perform copy elision
Yeah, what you suggested is what I tried on a previous patch in this DR, but 
then studying the NRVO tracker carefully I thought about this counter example:
```
template static auto bar() {
  { 
Foo foo;
if constexpr(B) 
  return foo;  
  } 
  { 
Bar bar;
if constexpr(!B)
  return bar;   
  } 
}   


Since we run the tracker before instantiation, we would see both return 
statements and mark both foo and bar as NRVO variables.
Ofcourse in the B = false case, we would end up constructing a Foo in a Bar 
return slot

As a side note, It is actually funny that we currently perform this 
optimization (most likely accidentally):
```
template static Foo bar() {
  { 
Foo foo1;
if constexpr(B) 
  return foo1;  
  } 
  {
Foo foo2;
return foo2 
  } 
}
```
In the B = false case, we end up constructing foo1 in the return slot even 
though we actually never return it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:4728
 
-  enum CopyElisionSemanticsKind {
-CES_Strict = 0,
-CES_AllowParameters = 1,
-CES_AllowDifferentTypes = 2,
-CES_AllowExceptionVariables = 4,
-CES_AllowRValueReferenceType = 8,
-CES_ImplicitlyMovableCXX11CXX14CXX17 =
-(CES_AllowParameters | CES_AllowDifferentTypes),
-CES_ImplicitlyMovableCXX20 =
-(CES_AllowParameters | CES_AllowDifferentTypes |
- CES_AllowExceptionVariables | CES_AllowRValueReferenceType),
+  struct NRVOResult {
+const VarDecl *Candidate;

We generally use `FooResult` to mean "either a `Foo` or an indicator that an 
error has been diagnosed". Would the name `NRVOInfo` work instead?

Actually, I think the "O" here is misleading, because we're not talking about 
performing the optimization here, just about what variable was named in a 
return statement. And I think calling this "NRVO" is a bit misleading because 
it also covers implicit move, which isn't traditionally part of NRVO. So how 
about `NamedReturnValueInfo` or `NamedReturnInfo` or `NamedReturnValue` or 
`ReturnValueName` or something like that?



Comment at: clang/lib/Sema/SemaStmt.cpp:3066
 
-  if (isCopyElisionCandidate(ReturnType, VD, CESK))
-return VD;
-  return nullptr;
+static void downgradeNRVOResult(Sema::NRVOResult , bool CanMove) {
+  Res.S = std::min(Res.S, CanMove ? Sema::NRVOResult::MoveEligible

I find this name a little unclear (what do we mean by "downgrade"?). Can we 
call this something a bit more specific, such as `disallowCopyElision` or 
`disallowNRVO`?



Comment at: clang/lib/Sema/SemaStmt.cpp:3138
+/// copy-elidable statuses, considering the function return type criteria
+/// as applicable to return statements.
+///

This comment needs re-flowing.



Comment at: clang/lib/Sema/SemaStmt.cpp:3144
+/// \returns The copy elision candidate, in case the initial return expression
+/// was copy elisible, or nullptr otherwise.
+const VarDecl *Sema::getCopyElisionCandidate(NRVOResult ,





Comment at: clang/lib/Sema/SemaStmt.cpp:3150-3153
+  // If we got a non-deduced auto ReturnType, we are in a dependent context and
+  // there is no point in allowing copy elision since we won't have it deduced
+  // by the point the VardDecl is instantiated, which is the last chance we 
have
+  // of deciding if the candidate is really copy elisible.

How does this happen? Are there any cases where we could do NRVO or should do 
an implicit move that are blocked by this?

It seems to me that we should (nearly always) be able to work out whether copy 
elision is possible here without knowing the deduced type:
-- if the return type is //cv// `auto` then it will always be deduced to the 
type of the returned variable, so we can always perform copy elision
-- if the return type is `decltype(auto)`, then we can perform copy elision if 
the expression is unparenthesized and otherwise cannot; we could perhaps track 
whether the expression was parenthesized in `NRVOResult`, and can 
conservatively disallow copy elision if we don't know (eg, from template 
instantiation, where we're only looking at the variable and not the return 
statements)
-- if the return type is anything else involving `auto`, it can't possibly 
instantiate to a class type, so we'll never perform copy elision



Comment at: clang/lib/Sema/SemaStmt.cpp:3152-3153
+  // there is no point in allowing copy elision since we won't have it deduced
+  // by the point the VardDecl is instantiated, which is the last chance we 
have
+  // of deciding if the candidate is really copy elisible.
+  if (AutoType *AT = ReturnType->getContainedAutoType())





Comment at: clang/lib/Sema/SemaStmt.cpp:3156
+if (AT->isCanonicalUnqualified())
+  return Res = NRVOResult(), nullptr;
+

Please add braces rather than using a comma expression here and below.



Comment at: clang/lib/Sema/SemaStmt.cpp:3290
 }
-
-if (!getLangOpts().CPlusPlus20 && NeedSecondOverloadResolution &&
-!getDiagnostics().isIgnored(diag::warn_return_std_move,
+if (!getDiagnostics().isIgnored(diag::warn_return_std_move,
 Value->getExprLoc())) {

Should we skip this if `NRVORes.isMoveEligible() && getLangOpts().CPlusPlus20`? 
In that case, we already tried an unrestricted move a couple of lines ago, so 
we know that adding `std::move` won't help. (I think we can still hit the 
"adding `std::move` will help" case in C++20, but only in obscure corners such 
as a `volatile`-qualified local variable.)



Comment at: clang/lib/Sema/SemaStmt.cpp:3470
+InitializedEntity Entity = InitializedEntity::InitializeResult(
+ReturnLoc, FnRetType, 

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov marked an inline comment as done.
mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3140
+/// \param ReturnType This is the return type of the function.
+void Sema::updNRVOResultWithRetType(NRVOResult , QualType ReturnType) {
+  if (!Res.Candidate)

Quuxplusone wrote:
> mizvekov wrote:
> > aaronpuchert wrote:
> > > mizvekov wrote:
> > > > aaronpuchert wrote:
> > > > > `NRVOResult` seems to be small, why not make this a proper function 
> > > > > and let it return the result?
> > > > It does use the result parameter as input and output. This function can 
> > > > only "downgrade" a result it received previously.
> > > > I could make it receive a result and return the result, but then would 
> > > > probably just use it like this:
> > > > ```
> > > > NRVORes = updNRVOResultWithRetType(NRVORes, ...);
> > > > ```
> > > > Do you think that looks clearer?
> > > Yes, that would seem more natural to me. Technically a caller could 
> > > decide to not use the same variable in both places, for example it could 
> > > pass a temporary or pass the result on directly.
> > But for this function, I don't see how it could make sense to actually use 
> > it like that.
> > 
> > You are either using it in a context where you don't have a ReturnType 
> > (throw, co_return), and you only care about the initial NRVOResult and 
> > don't call this function at all, or you are in a return statement, where 
> > you will call this function passing the initial NRVOResult, which will then 
> > have no further utility.
> I haven't looked at the code, but FWIW, I'm always in favor of more 
> "function-style" functions. Which would you rather deal with — `void 
> square(int& x)` or `int square(int x)`? The same rationale applies uniformly 
> everywhere. Even if we are "consuming" the argument in this particular 
> instance, that might change after another few years of maintenance; and 
> regardless, it might be easier for the maintainer to understand 
> "function-style" code even if they aren't going to change it.
I changed the function name and made it return instead the candidate itself in 
case it was copy elisible, which allowed simplifying some other expressions 
afterwards.

But in answer to the points made in this thread: I will generally prefer the 
syntax which mostly matches the intended usage, making it easier to do the 
right thing and harder to do the wrong thing.

This function implements a fragment of the C++ standard text in a very specific 
context within clang. It is called from two places.
It's not a major piece of infrastructure or anything like that, to be concerned 
with unforeseen possible future uses :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov updated this revision to Diff 336736.
mizvekov marked an inline comment as done.
mizvekov added a comment.

- Removes code that tried to allow copy elision for functions with dependent 
'auto' return types. The reason is explained in new comments. Will try to 
address these in future work.
- Addresses aaronpuchert's review points.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/nrvo-tracking.cpp

Index: clang/test/CodeGen/nrvo-tracking.cpp
===
--- clang/test/CodeGen/nrvo-tracking.cpp
+++ clang/test/CodeGen/nrvo-tracking.cpp
@@ -29,8 +29,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(3, t, T);
@@ -152,7 +150,13 @@
   }; }()();\
 }
 
-//B(1, X); // Uncomment this line at your own peril ;)
+// CHECK-LABEL: define{{.*}} void @_Z2b1v
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+B(1, X);
 
 // CHECK-LABEL: define{{.*}} void @_Z2b2v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1050,11 +1050,29 @@
 
   SemaRef.BuildVariableInstantiation(Var, D, TemplateArgs, LateAttrs, Owner,
  StartingScope, InstantiatingVarTemplate);
-
   if (D->isNRVOVariable()) {
-QualType ReturnType = cast(DC)->getReturnType();
-if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
-  Var->setNRVOVariable(true);
+QualType ReturnType;
+if (auto *F = dyn_cast(DC))
+  ReturnType = F->getReturnType();
+else if (auto *F = dyn_cast(DC))
+  // FIXME: get the return type here somehow...
+  ReturnType = SemaRef.Context.getAutoDeductType();
+else
+  llvm_unreachable("Unknown context type");
+
+// This is the last chance we have of checking copy elision eligibility
+// for functions in depdendent contexts. The sema actions for building
+// the return statement during template instantiation will have no effect
+// regarding copy elision, since NRVO propagation runs on the scope exit
+// actions, and these are not run on instantiation.
+// This might run through some VarDecls which were returned from non-taken
+// 'if constexpr' branches, and these will end up being constructed on the
+// return slot even if they will never be returned, as a sort of accidental
+// 'optimization'. Notably, functions with 'auto' return types won't have it
+// deduced by this point. Coupled with the limitation described
+// previously, this makes it very hard to support copy elision for these.
+Sema::NRVOResult Res = SemaRef.getNRVOResult(Var);
+Var->setNRVOVariable(!!SemaRef.getCopyElisionCandidate(Res, ReturnType));
   }
 
   Var->setImplicit(D->isImplicit());
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3036,99 +3036,139 @@
   return new (Context) BreakStmt(BreakLoc);
 }
 
-/// Determine whether the given expression is a candidate for
-/// copy elision in either a return statement or a throw expression.
+/// Determine whether the given expression might be move-eligible or
+/// copy-elidable in either a (co_)return statement or throw expression,
+/// without considering function return type, if applicable.
 ///
-/// \param ReturnType If we're determining the copy elision candidate for
-/// a return statement, this is the return type of the function. If we're
-/// determining the copy elision candidate for a throw expression, this will
-/// be a NULL type.
+/// \param E The expression being returned from the function or block,
+/// being thrown, or being co_returned from a coroutine.
 ///
-/// \param E The expression being returned from the function or block, or
-/// being thrown.
+/// \param ForceCXX20 Overrides detection of current language mode
+/// and uses the rules for C++20.
 ///
-/// \param CESK Whether we allow function parameters or
-/// id-expressions that could be moved out of the function to be considered NRVO
-/// candidates. C++ prohibits these for NRVO itself, but we re-use this logic 

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:1585-1586
   InitializedEntity Entity = InitializedEntity::InitializeVariable(GroDecl);
-  ExprResult Res = S.PerformMoveOrCopyInitialization(Entity, nullptr, GroType,
- this->ReturnValue);
+  ExprResult Res =
+  S.PerformCopyInitialization(Entity, SourceLocation(), ReturnValue);
   if (Res.isInvalid())

mizvekov wrote:
> mizvekov wrote:
> > mizvekov wrote:
> > > aaronpuchert wrote:
> > > > Perhaps this should just be direct initialization? Can't really find 
> > > > anything in the standard though.
> > > So I just checked this again. Regarding our conversation on IRC, what I 
> > > said initially was correct: `ReturnValue` is always a member function 
> > > expression, built by `makeReturnObject` -> `buildPromiseCall` -> 
> > > `buildMemberCall`. So implicit move would never trigger here, and as far 
> > > as I see there is no reason for this code to have used 
> > > PerformMoveOrCopyInitialization in the first place.
> > Err I meant: member function *call* expression
> Also, I don't think we could force direct initialization here, if the object 
> returned by Gro is volatile for example.
> With that said, copy elision from the sema action on the return statement of 
> get_return_object should take care of removing this copy here, I think, I 
> don't see any reason it would not work here just as well as for expressions 
> coming from the parser.
In `InitializationKind` there is a distinction between `IK_Copy` (basically `T 
x = init;`) and `IK_Direct` (basically `T x(init);`). I don't know how this 
would be related to `volatile`, but it is related to `explicit`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

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

Quuxplusone added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3140
+/// \param ReturnType This is the return type of the function.
+void Sema::updNRVOResultWithRetType(NRVOResult , QualType ReturnType) {
+  if (!Res.Candidate)

mizvekov wrote:
> aaronpuchert wrote:
> > mizvekov wrote:
> > > aaronpuchert wrote:
> > > > `NRVOResult` seems to be small, why not make this a proper function and 
> > > > let it return the result?
> > > It does use the result parameter as input and output. This function can 
> > > only "downgrade" a result it received previously.
> > > I could make it receive a result and return the result, but then would 
> > > probably just use it like this:
> > > ```
> > > NRVORes = updNRVOResultWithRetType(NRVORes, ...);
> > > ```
> > > Do you think that looks clearer?
> > Yes, that would seem more natural to me. Technically a caller could decide 
> > to not use the same variable in both places, for example it could pass a 
> > temporary or pass the result on directly.
> But for this function, I don't see how it could make sense to actually use it 
> like that.
> 
> You are either using it in a context where you don't have a ReturnType 
> (throw, co_return), and you only care about the initial NRVOResult and don't 
> call this function at all, or you are in a return statement, where you will 
> call this function passing the initial NRVOResult, which will then have no 
> further utility.
I haven't looked at the code, but FWIW, I'm always in favor of more 
"function-style" functions. Which would you rather deal with — `void 
square(int& x)` or `int square(int x)`? The same rationale applies uniformly 
everywhere. Even if we are "consuming" the argument in this particular 
instance, that might change after another few years of maintenance; and 
regardless, it might be easier for the maintainer to understand 
"function-style" code even if they aren't going to change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3140
+/// \param ReturnType This is the return type of the function.
+void Sema::updNRVOResultWithRetType(NRVOResult , QualType ReturnType) {
+  if (!Res.Candidate)

aaronpuchert wrote:
> mizvekov wrote:
> > aaronpuchert wrote:
> > > `NRVOResult` seems to be small, why not make this a proper function and 
> > > let it return the result?
> > It does use the result parameter as input and output. This function can 
> > only "downgrade" a result it received previously.
> > I could make it receive a result and return the result, but then would 
> > probably just use it like this:
> > ```
> > NRVORes = updNRVOResultWithRetType(NRVORes, ...);
> > ```
> > Do you think that looks clearer?
> Yes, that would seem more natural to me. Technically a caller could decide to 
> not use the same variable in both places, for example it could pass a 
> temporary or pass the result on directly.
But for this function, I don't see how it could make sense to actually use it 
like that.

You are either using it in a context where you don't have a ReturnType (throw, 
co_return), and you only care about the initial NRVOResult and don't call this 
function at all, or you are in a return statement, where you will call this 
function passing the initial NRVOResult, which will then have no further 
utility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3030-3032
+  Res.S = Res.S != Sema::NRVOResult::None && CanMove
+  ? Sema::NRVOResult::MoveEligible
+  : Sema::NRVOResult::None;

mizvekov wrote:
> aaronpuchert wrote:
> > The way you've designed the enumeration, couldn't you use `std::min` here?
> Yes exactly!
> The previous version of this patch I submitted used exactly that, but then 
> for some reason I thought people could find std::min a bit odd here, and so I 
> changed it.
> But now I think I was right the first time around :)
Perhaps you can just add a comment to the enumeration saying that the values 
are totally ordered with regards to implication so that ⇒ is >=.



Comment at: clang/lib/Sema/SemaStmt.cpp:3058
 
-bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
-  CopyElisionSemanticsKind CESK) {
-  QualType VDType = VD->getType();
-  // - in a return statement in a function with ...
-  // ... a class return type ...
-  if (!ReturnType.isNull() && !ReturnType->isDependentType()) {
-if (!ReturnType->isRecordType())
-  return false;
-// ... the same cv-unqualified type as the function return type ...
-// When considering moving this expression out, allow dissimilar types.
-if (!(CESK & CES_AllowDifferentTypes) && !VDType->isDependentType() &&
-!Context.hasSameUnqualifiedType(ReturnType, VDType))
-  return false;
+  // (other than a function ... parameter)
+  if (VD->getKind() == Decl::ParmVar) {

mizvekov wrote:
> aaronpuchert wrote:
> > These comments seem to be separated from their context now...
> This one comment here is talking about the line just below it.
> Any other examples where it seems separated?
I meant the context of the comment itself. Originally this was quoting a clause

```
  // - in a return statement in a function [where] ...
  // ... the expression is the name of a non-volatile automatic object ...
```

and then additional parts later. Now you're having that initial part in a 
different function and it's not immediately clear what you're quoting here.

To be clear, this was about "(other than a function ... parameter)" below.



Comment at: clang/lib/Sema/SemaStmt.cpp:3140
+/// \param ReturnType This is the return type of the function.
+void Sema::updNRVOResultWithRetType(NRVOResult , QualType ReturnType) {
+  if (!Res.Candidate)

mizvekov wrote:
> aaronpuchert wrote:
> > `NRVOResult` seems to be small, why not make this a proper function and let 
> > it return the result?
> It does use the result parameter as input and output. This function can only 
> "downgrade" a result it received previously.
> I could make it receive a result and return the result, but then would 
> probably just use it like this:
> ```
> NRVORes = updNRVOResultWithRetType(NRVORes, ...);
> ```
> Do you think that looks clearer?
Yes, that would seem more natural to me. Technically a caller could decide to 
not use the same variable in both places, for example it could pass a temporary 
or pass the result on directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov marked 8 inline comments as done.
mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:3058
 
-bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
-  CopyElisionSemanticsKind CESK) {
-  QualType VDType = VD->getType();
-  // - in a return statement in a function with ...
-  // ... a class return type ...
-  if (!ReturnType.isNull() && !ReturnType->isDependentType()) {
-if (!ReturnType->isRecordType())
-  return false;
-// ... the same cv-unqualified type as the function return type ...
-// When considering moving this expression out, allow dissimilar types.
-if (!(CESK & CES_AllowDifferentTypes) && !VDType->isDependentType() &&
-!Context.hasSameUnqualifiedType(ReturnType, VDType))
-  return false;
+  // (other than a function ... parameter)
+  if (VD->getKind() == Decl::ParmVar) {

aaronpuchert wrote:
> These comments seem to be separated from their context now...
This one comment here is talking about the line just below it.
Any other examples where it seems separated?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov updated this revision to Diff 336626.
mizvekov added a comment.

- Addresses aaronpuchert's review points.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/nrvo-tracking.cpp

Index: clang/test/CodeGen/nrvo-tracking.cpp
===
--- clang/test/CodeGen/nrvo-tracking.cpp
+++ clang/test/CodeGen/nrvo-tracking.cpp
@@ -29,8 +29,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(3, t, T);
@@ -45,8 +43,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l5v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(5, t, auto);
@@ -61,8 +57,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l7v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(7, t, decltype(auto));
@@ -113,8 +107,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2f5v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 F(5, t, auto);
@@ -129,8 +121,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2f7v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 F(7, t, decltype(auto));
@@ -152,7 +142,11 @@
   }; }()();\
 }
 
-//B(1, X); // Uncomment this line at your own peril ;)
+// CHECK-LABEL: define{{.*}} void @_Z2b1v
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+B(1, X);
 
 // CHECK-LABEL: define{{.*}} void @_Z2b2v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
@@ -164,8 +158,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(3, T);
@@ -180,8 +172,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b5v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(5, );
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1052,9 +1052,18 @@
  StartingScope, InstantiatingVarTemplate);
 
   if (D->isNRVOVariable()) {
-QualType ReturnType = cast(DC)->getReturnType();
-if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
-  Var->setNRVOVariable(true);
+QualType ReturnType;
+if (auto *F = dyn_cast(DC))
+  ReturnType = F->getReturnType();
+else if (auto *F = dyn_cast(DC))
+  // FIXME: get the return type here somehow...
+  ReturnType = SemaRef.Context.getAutoDeductType();
+else
+  llvm_unreachable("Unknown context type");
+
+Sema::NRVOResult Res = SemaRef.getNRVOResult(Var);
+SemaRef.updNRVOResultWithRetType(Res, ReturnType);
+Var->setNRVOVariable(Res.isCopyElidable());
   }
 
   Var->setImplicit(D->isImplicit());
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3036,99 +3036,152 @@
   return new (Context) BreakStmt(BreakLoc);
 }
 
-/// Determine whether the given expression is a candidate for
-/// copy elision in either a return statement or a throw expression.
+/// Determine whether the given expression might be move-eligible or
+/// copy-elidable in either a (co_)return statement or throw expression,
+/// without considering function return type, if applicable.
 ///
-/// \param ReturnType If we're determining the copy elision candidate for
-/// a return statement, this is the return type of the function. If we're
-/// determining the copy elision candidate for a throw expression, this will
-/// be a NULL type.
+/// \param E The expression being returned from the function or block,

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:1585-1586
   InitializedEntity Entity = InitializedEntity::InitializeVariable(GroDecl);
-  ExprResult Res = S.PerformMoveOrCopyInitialization(Entity, nullptr, GroType,
- this->ReturnValue);
+  ExprResult Res =
+  S.PerformCopyInitialization(Entity, SourceLocation(), ReturnValue);
   if (Res.isInvalid())

mizvekov wrote:
> mizvekov wrote:
> > aaronpuchert wrote:
> > > Perhaps this should just be direct initialization? Can't really find 
> > > anything in the standard though.
> > So I just checked this again. Regarding our conversation on IRC, what I 
> > said initially was correct: `ReturnValue` is always a member function 
> > expression, built by `makeReturnObject` -> `buildPromiseCall` -> 
> > `buildMemberCall`. So implicit move would never trigger here, and as far as 
> > I see there is no reason for this code to have used 
> > PerformMoveOrCopyInitialization in the first place.
> Err I meant: member function *call* expression
Also, I don't think we could force direct initialization here, if the object 
returned by Gro is volatile for example.
With that said, copy elision from the sema action on the return statement of 
get_return_object should take care of removing this copy here, I think, I don't 
see any reason it would not work here just as well as for expressions coming 
from the parser.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:854
   if (Ex && !Ex->isTypeDependent()) {
+NRVOResult NRVORes = IsThrownVarInScope ? getNRVOResult(Ex) : NRVOResult();
+

mizvekov wrote:
> mizvekov wrote:
> > aaronpuchert wrote:
> > > Any reason why you've moved that away from the comment that wants to 
> > > explain it?
> > Yes, on C++2b this might modify Ex, and I moved it so this would happen 
> > before we do the check with the type of the expression just below here.
> But yeah I forgot to move the comment, good catch.
Actually, had my wires crossed there with the P2266 DR, I only need to move 
this line there.



Comment at: clang/lib/Sema/SemaStmt.cpp:3157-3159
+  // it would deduce to a reference.
+  if (AT->isDecltypeAuto() && Res.isParenthesized)
+return Res = NRVOResult(), void();

aaronpuchert wrote:
> Can't we just do this when we know what it deduces to? It sounds weird to 
> handle dependent contexts here because we shouldn't attempt any return value 
> initialization then.
I think there are problems regarding the limitations of the current NRVO 
algorithm, but I had not had much time to study it or think of improvements, so 
I follow the current approach of trying to use as much available information as 
possible in order to eliminate candidates early.

With that said, this is not well covered by the test suite and I could probably 
get away with a lot less without breaking any existing tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:854
   if (Ex && !Ex->isTypeDependent()) {
+NRVOResult NRVORes = IsThrownVarInScope ? getNRVOResult(Ex) : NRVOResult();
+

mizvekov wrote:
> aaronpuchert wrote:
> > Any reason why you've moved that away from the comment that wants to 
> > explain it?
> Yes, on C++2b this might modify Ex, and I moved it so this would happen 
> before we do the check with the type of the expression just below here.
But yeah I forgot to move the comment, good catch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:1585-1586
   InitializedEntity Entity = InitializedEntity::InitializeVariable(GroDecl);
-  ExprResult Res = S.PerformMoveOrCopyInitialization(Entity, nullptr, GroType,
- this->ReturnValue);
+  ExprResult Res =
+  S.PerformCopyInitialization(Entity, SourceLocation(), ReturnValue);
   if (Res.isInvalid())

mizvekov wrote:
> aaronpuchert wrote:
> > Perhaps this should just be direct initialization? Can't really find 
> > anything in the standard though.
> So I just checked this again. Regarding our conversation on IRC, what I said 
> initially was correct: `ReturnValue` is always a member function expression, 
> built by `makeReturnObject` -> `buildPromiseCall` -> `buildMemberCall`. So 
> implicit move would never trigger here, and as far as I see there is no 
> reason for this code to have used PerformMoveOrCopyInitialization in the 
> first place.
Err I meant: member function *call* expression


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:1585-1586
   InitializedEntity Entity = InitializedEntity::InitializeVariable(GroDecl);
-  ExprResult Res = S.PerformMoveOrCopyInitialization(Entity, nullptr, GroType,
- this->ReturnValue);
+  ExprResult Res =
+  S.PerformCopyInitialization(Entity, SourceLocation(), ReturnValue);
   if (Res.isInvalid())

aaronpuchert wrote:
> Perhaps this should just be direct initialization? Can't really find anything 
> in the standard though.
So I just checked this again. Regarding our conversation on IRC, what I said 
initially was correct: `ReturnValue` is always a member function expression, 
built by `makeReturnObject` -> `buildPromiseCall` -> `buildMemberCall`. So 
implicit move would never trigger here, and as far as I see there is no reason 
for this code to have used PerformMoveOrCopyInitialization in the first place.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:854
   if (Ex && !Ex->isTypeDependent()) {
+NRVOResult NRVORes = IsThrownVarInScope ? getNRVOResult(Ex) : NRVOResult();
+

aaronpuchert wrote:
> Any reason why you've moved that away from the comment that wants to explain 
> it?
Yes, on C++2b this might modify Ex, and I moved it so this would happen before 
we do the check with the type of the expression just below here.



Comment at: clang/lib/Sema/SemaStmt.cpp:3030-3032
+  Res.S = Res.S != Sema::NRVOResult::None && CanMove
+  ? Sema::NRVOResult::MoveEligible
+  : Sema::NRVOResult::None;

aaronpuchert wrote:
> The way you've designed the enumeration, couldn't you use `std::min` here?
Yes exactly!
The previous version of this patch I submitted used exactly that, but then for 
some reason I thought people could find std::min a bit odd here, and so I 
changed it.
But now I think I was right the first time around :)



Comment at: clang/lib/Sema/SemaStmt.cpp:3140
+/// \param ReturnType This is the return type of the function.
+void Sema::updNRVOResultWithRetType(NRVOResult , QualType ReturnType) {
+  if (!Res.Candidate)

aaronpuchert wrote:
> `NRVOResult` seems to be small, why not make this a proper function and let 
> it return the result?
It does use the result parameter as input and output. This function can only 
"downgrade" a result it received previously.
I could make it receive a result and return the result, but then would probably 
just use it like this:
```
NRVORes = updNRVOResultWithRetType(NRVORes, ...);
```
Do you think that looks clearer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Didn't really check for correctness yet, just a superficial review.

I like the idea, splitting the functionality up definitely helps understanding 
this.




Comment at: clang/include/clang/Sema/Sema.h:4733
+
+bool isMoveEligible() const { return S >= MoveEligible; };
+bool isCopyElidable() const { return S == MoveEligibleAndCopyElidable; }

Nitpick: I'd go with `!= None` here.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:1585-1586
   InitializedEntity Entity = InitializedEntity::InitializeVariable(GroDecl);
-  ExprResult Res = S.PerformMoveOrCopyInitialization(Entity, nullptr, GroType,
- this->ReturnValue);
+  ExprResult Res =
+  S.PerformCopyInitialization(Entity, SourceLocation(), ReturnValue);
   if (Res.isInvalid())

Perhaps this should just be direct initialization? Can't really find anything 
in the standard though.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:854
   if (Ex && !Ex->isTypeDependent()) {
+NRVOResult NRVORes = IsThrownVarInScope ? getNRVOResult(Ex) : NRVOResult();
+

Any reason why you've moved that away from the comment that wants to explain it?



Comment at: clang/lib/Sema/SemaStmt.cpp:3030-3032
+  Res.S = Res.S != Sema::NRVOResult::None && CanMove
+  ? Sema::NRVOResult::MoveEligible
+  : Sema::NRVOResult::None;

The way you've designed the enumeration, couldn't you use `std::min` here?



Comment at: clang/lib/Sema/SemaStmt.cpp:3040
 ///
-/// \param E The expression being returned from the function or block, or
-/// being thrown.
+/// \\param Parenthesized Whether the expression this candidate was obtained
+/// from was parenthesized.

The second backslash seems superfluous.



Comment at: clang/lib/Sema/SemaStmt.cpp:3058
 
-bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
-  CopyElisionSemanticsKind CESK) {
-  QualType VDType = VD->getType();
-  // - in a return statement in a function with ...
-  // ... a class return type ...
-  if (!ReturnType.isNull() && !ReturnType->isDependentType()) {
-if (!ReturnType->isRecordType())
-  return false;
-// ... the same cv-unqualified type as the function return type ...
-// When considering moving this expression out, allow dissimilar types.
-if (!(CESK & CES_AllowDifferentTypes) && !VDType->isDependentType() &&
-!Context.hasSameUnqualifiedType(ReturnType, VDType))
-  return false;
+  // (other than a function ... parameter)
+  if (VD->getKind() == Decl::ParmVar) {

These comments seem to be separated from their context now...



Comment at: clang/lib/Sema/SemaStmt.cpp:3059
+  // (other than a function ... parameter)
+  if (VD->getKind() == Decl::ParmVar) {
+downgradeNRVOResult(Res, hasCXX11);

Braces are usually omitted if both branches are single statements.



Comment at: clang/lib/Sema/SemaStmt.cpp:3073
 
   // Return false if VD is a __block variable. We don't want to implicitly move
   // out of a __block variable during a return because we cannot assume the

Just remove "Return false if VD is a __block variable." No need to repeat the 
code, what follows is important.



Comment at: clang/lib/Sema/SemaStmt.cpp:3107
+
+/// Determine whether the given expression might be move-eligible or
+/// copy-elidable in either a (co_)return statement or throw expression,

Can we maybe move this function up one place to make the diff a bit smaller? It 
appears to contain the first part of the original `getCopyElisionCandidate`.



Comment at: clang/lib/Sema/SemaStmt.cpp:3140
+/// \param ReturnType This is the return type of the function.
+void Sema::updNRVOResultWithRetType(NRVOResult , QualType ReturnType) {
+  if (!Res.Candidate)

`NRVOResult` seems to be small, why not make this a proper function and let it 
return the result?



Comment at: clang/lib/Sema/SemaStmt.cpp:3156
+  // If is decltype(auto) and the original return expression
+  // was parethesized, then we can discard this candidate because
+  // it would deduce to a reference.

parenthesized



Comment at: clang/lib/Sema/SemaStmt.cpp:3157-3159
+  // it would deduce to a reference.
+  if (AT->isDecltypeAuto() && Res.isParenthesized)
+return Res = NRVOResult(), void();

Can't we just do this when we know what it deduces to? It sounds weird to 
handle dependent contexts here because we shouldn't attempt any return value 
initialization then.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1062
+else
+  assert(false && "Unknown 

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov updated this revision to Diff 335532.
mizvekov added a comment.

- small Doxygen fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/nrvo-tracking.cpp

Index: clang/test/CodeGen/nrvo-tracking.cpp
===
--- clang/test/CodeGen/nrvo-tracking.cpp
+++ clang/test/CodeGen/nrvo-tracking.cpp
@@ -29,8 +29,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(3, t, T);
@@ -45,8 +43,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l5v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(5, t, auto);
@@ -61,8 +57,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2l7v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 L(7, t, decltype(auto));
@@ -113,8 +107,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2f5v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 F(5, t, auto);
@@ -129,8 +121,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2f7v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 F(7, t, decltype(auto));
@@ -152,7 +142,11 @@
   }; }()();\
 }
 
-//B(1, X); // Uncomment this line at your own peril ;)
+// CHECK-LABEL: define{{.*}} void @_Z2b1v
+// CHECK:   call {{.*}} @_ZN1XC1Ev
+// CHECK-NEXT:  call void @llvm.lifetime.end
+// CHECK-NEXT:  ret void
+B(1, X);
 
 // CHECK-LABEL: define{{.*}} void @_Z2b2v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
@@ -164,8 +158,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b3v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(3, T);
@@ -180,8 +172,6 @@
 
 // CHECK-LABEL: define{{.*}} void @_Z2b5v
 // CHECK:   call {{.*}} @_ZN1XC1Ev
-// CHECK-NEXT:  call {{.*}} @_ZN1XC1EOS_
-// CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  call void @llvm.lifetime.end
 // CHECK-NEXT:  ret void
 B(5, );
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1052,9 +1052,18 @@
  StartingScope, InstantiatingVarTemplate);
 
   if (D->isNRVOVariable()) {
-QualType ReturnType = cast(DC)->getReturnType();
-if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
-  Var->setNRVOVariable(true);
+QualType ReturnType;
+if (auto *F = dyn_cast(DC))
+  ReturnType = F->getReturnType();
+else if (auto *F = dyn_cast(DC))
+  // FIXME: get the return type here somehow...
+  ReturnType = SemaRef.Context.getAutoDeductType();
+else
+  assert(false && "Unknown context type");
+
+Sema::NRVOResult Res = SemaRef.getNRVOResult(Var);
+SemaRef.updNRVOResultWithRetType(Res, ReturnType);
+Var->setNRVOVariable(Res.isCopyElidable());
   }
 
   Var->setImplicit(D->isImplicit());
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3026,99 +3026,155 @@
   return new (Context) BreakStmt(BreakLoc);
 }
 
-/// Determine whether the given expression is a candidate for
-/// copy elision in either a return statement or a throw expression.
+static void downgradeNRVOResult(Sema::NRVOResult , bool CanMove) {
+  Res.S = Res.S != Sema::NRVOResult::None && CanMove
+  ? Sema::NRVOResult::MoveEligible
+  : Sema::NRVOResult::None;
+}
+
+/// Determine whether the given NRVO candidate variable is move-eligible or
+/// copy-elidable, without considering function return type.
 ///
-/// \param ReturnType If we're determining the copy elision candidate for
-/// a return statement, this is the return type of the function. If we're
-/// determining the copy elision candidate