[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-20 Thread David Tenty via Phabricator via cfe-commits 
daltenty added a comment.

Thanks for getting this fixed this so quickly and sorry for the messy revert.

> What you're seeing is a symptom of a libc++ issue (PR12704) that was fixed 
> >10 years ago (rGbff1bfc6be0615ba3036a861fd27b75c96e3297c 
> ). I 
> don't think we have explicit rules on how old a version of libc++ we aim to 
> support, but trying to use version 3.1 or earlier with trunk Clang seems at 
> least a little unreasonable.  That said, it's probably not too hard to work 
> around this, so I will :) but I would encourage whoever is responsible for 
> libc++ in AIX to look into an upgrade!

Yeah, I'd tend to agree with you :) Thanks for accommodating us and identifying 
the libc++ issue as well, we'll definitely be looking into this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-20 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-20 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

In D123345#3463540 , @thakis wrote:

> I think your revert to fix the aix CI broke CI everywhere: 
> http://45.33.8.238/linux/74239/step_7.txt
>
> Please take a look and reland for now if it takes a while to fix.

Relanded with a workaround for super-old libc++ in 
rG72315d02c432a0fe0acae9c96c69eac8d8e1a9f6 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-20 Thread Nico Weber via Phabricator via cfe-commits 
thakis added a comment.

I think your revert to fix the aix CI broke CI everywhere: 
http://45.33.8.238/linux/74239/step_7.txt

Please take a look and reland for now if it takes a while to fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-20 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

In D123345#3463224 , @daltenty wrote:

> In D123345#3460639 , @Jake-Egan 
> wrote:
>
>> Hi, unfortunately there's a build failure on AIX: 
>> https://lab.llvm.org/buildbot/#/builders/214/builds/779/steps/9/logs/stdio. 
>> Could you take a look?
>
> Hi, any feedback on the AIX break? Seems like this might be non-trivial to 
> address (were on an older libc++ level), so perhaps this needs a revert till 
> can be worked out.

Sorry, I somehow missed this. What you're seeing is a symptom of a libc++ issue 
(PR12704) that was fixed >10 years ago 
(rGbff1bfc6be0615ba3036a861fd27b75c96e3297c 
). I don't 
think we have explicit rules on how old a version of libc++ we aim to support, 
but trying to use version 3.1 or earlier with trunk Clang seems at least a 
little unreasonable. That said, it's probably not too hard to work around this, 
so I will :) but I would encourage whoever is responsible for libc++ in AIX to 
look into an upgrade!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-20 Thread David Tenty via Phabricator via cfe-commits 
daltenty added a comment.

In D123345#3460639 , @Jake-Egan wrote:

> Hi, unfortunately there's a build failure on AIX: 
> https://lab.llvm.org/buildbot/#/builders/214/builds/779/steps/9/logs/stdio. 
> Could you take a look?

Hi, any feedback on the AIX break? Seems like this might be non-trivial to 
address (were on an older libc++ level), so perhaps this needs a revert till 
can be worked out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-19 Thread Jake Egan via Phabricator via cfe-commits 
Jake-Egan added a comment.

Hi, unfortunately there's a build failure on AIX: 
https://lab.llvm.org/buildbot/#/builders/214/builds/779/steps/9/logs/stdio. 
Could you take a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-18 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

In D123345#3457109 , @rsmith wrote:

> I'm working on a fix. I'll revert if it takes me more than a few minutes. If 
> you'd prefer to not wait, please feel free to revert it yourself.

Thanks for your patience,  should be fixed in 
rGe43c93dd63cca295ef26ab69cd305816a71d45fd 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-18 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

In D123345#3456964 , @paulkirth wrote:

> Hi, this is also breaking Fuchsia's clang CI builders 
> (https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8816531831869640417/overview).
>  If this will be hard to address, would you mind reverting until a patch is 
> ready?

I'm working on a fix. I'll revert if it takes me more than a few minutes. If 
you'd prefer to not wait, please feel free to revert it yourself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-18 Thread Paul Kirth via Phabricator via cfe-commits 
paulkirth added a comment.

Hi, this is also breaking Fuchsia's clang CI builders 
(https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8816531831869640417/overview).
 If this will be hard to address, would you mind reverting until a patch is 
ready?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-18 Thread Vitaly Buka via Phabricator via cfe-commits 
vitalybuka added a comment.

It's broken again here https://lab.llvm.org/buildbot/#/builders/74/builds/10427


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-17 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb27430f9f46b: Treat `std::move`, `forward`, etc. as 
builtins. (authored by rsmith).

Changed prior to commit:
  https://reviews.llvm.org/D123345?vs=423165=423312#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

Files:
  clang/docs/CommandGuide/clang.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/Builtins.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Basic/Builtins.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Analysis/inner-pointer.cpp
  clang/test/Analysis/use-after-move.cpp
  clang/test/CodeGenCXX/builtin-std-move.cpp
  clang/test/CodeGenCXX/builtins.cpp
  clang/test/CodeGenCXX/microsoft-abi-throw.cpp
  clang/test/SemaCXX/builtin-std-move-nobuiltin.cpp
  clang/test/SemaCXX/builtin-std-move.cpp
  clang/test/SemaCXX/unqualified-std-call-fixits.cpp
  clang/test/SemaCXX/unqualified-std-call.cpp
  clang/test/SemaCXX/warn-consumed-analysis.cpp
  clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

Index: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -1444,7 +1444,7 @@
 TEST(ExprMutationAnalyzerTest, ReproduceFailureMinimal) {
   const std::string Reproducer =
   "namespace std {"
-  "template  T forward(T & A) { return static_cast(A); }"
+  "template  T (T ) { return static_cast(A); }"
   "template  struct __bind {"
   "  T f;"
   "  template  __bind(T v, V &&) : f(forward(v)) {}"
Index: clang/test/SemaCXX/warn-consumed-analysis.cpp
===
--- clang/test/SemaCXX/warn-consumed-analysis.cpp
+++ clang/test/SemaCXX/warn-consumed-analysis.cpp
@@ -953,12 +953,12 @@
 namespace std {
   void move();
   template
-  void move(T&&);
+  T &(T&);
 
   namespace __1 {
 void move();
 template
-void move(T&&);
+T &(T&);
   }
 }
 
@@ -971,7 +971,7 @@
   void test() {
 x.move();
 std::move();
-std::move(x);
+std::move(x); // expected-warning {{ignoring return value}}
 std::__1::move();
 std::__1::move(x);
   }
Index: clang/test/SemaCXX/unqualified-std-call.cpp
===
--- clang/test/SemaCXX/unqualified-std-call.cpp
+++ clang/test/SemaCXX/unqualified-std-call.cpp
@@ -1,17 +1,17 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s -Wno-unused-value
 
 namespace std {
 
 template 
 void dummy(T &&) {}
 template 
-void move(T &&) {}
+T &(T &) { return x; }
 template 
 void move(T &&, U &&) {}
 
 inline namespace __1 {
 template 
-void forward(T &) {}
+T (T ) { return x; }
 } // namespace __1
 
 struct foo {};
Index: clang/test/SemaCXX/unqualified-std-call-fixits.cpp
===
--- clang/test/SemaCXX/unqualified-std-call-fixits.cpp
+++ clang/test/SemaCXX/unqualified-std-call-fixits.cpp
@@ -6,9 +6,9 @@
 
 namespace std {
 
-void move(auto &) {}
+int &(auto &) { return a; }
 
-void forward(auto ) {}
+int &(auto ) { return a; }
 
 } // namespace std
 
@@ -16,8 +16,8 @@
 
 void f() {
   int i = 0;
-  move(i); // expected-warning {{unqualified call to std::move}}
-  // CHECK: {{^}}  std::
-  forward(i); // expected-warning {{unqualified call to std::forward}}
-  // CHECK: {{^}}  std::
+  (void)move(i); // expected-warning {{unqualified call to std::move}}
+  // CHECK: {{^}}  (void)std::move
+  (void)forward(i); // expected-warning {{unqualified call to std::forward}}
+  // CHECK: {{^}}  (void)std::forward
 }
Index: clang/test/SemaCXX/builtin-std-move.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtin-std-move.cpp
@@ -0,0 +1,126 @@
+// RUN: %clang_cc1 -std=c++17 -verify %s
+// RUN: %clang_cc1 -std=c++17 -verify %s -DNO_CONSTEXPR
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+namespace std {
+#ifndef NO_CONSTEXPR
+#define CONSTEXPR constexpr
+#else
+#define CONSTEXPR
+#endif
+
+  template CONSTEXPR T &(T ) {
+static_assert(T::moveable, "instantiated move"); // expected-error {{no member named 'moveable' in 'B'}}
+ // expected-error@-1 {{no member named 'moveable' in 'C'}}
+return static_cast(x);
+  }
+
+

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-16 Thread Vitaly Buka via Phabricator via cfe-commits 
vitalybuka reopened this revision.
vitalybuka added a comment.
This revision is now accepted and ready to land.

Reverted with e75d8b70370435b0ad10388afba0df45fcf9bfcc 
 to 
recover bots


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-15 Thread Richard Smith - zygoloid 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 rG64c045e25b84: Treat `std::move`, `forward`, and 
`move_if_noexcept` as builtins. (authored by rsmith).

Changed prior to commit:
  https://reviews.llvm.org/D123345?vs=423164=423165#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

Files:
  clang/docs/CommandGuide/clang.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/Builtins.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Basic/Builtins.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Analysis/use-after-move.cpp
  clang/test/CodeGenCXX/builtin-std-move.cpp
  clang/test/CodeGenCXX/microsoft-abi-throw.cpp
  clang/test/SemaCXX/builtin-std-move-nobuiltin.cpp
  clang/test/SemaCXX/builtin-std-move.cpp
  clang/test/SemaCXX/unqualified-std-call-fixits.cpp
  clang/test/SemaCXX/unqualified-std-call.cpp
  clang/test/SemaCXX/warn-consumed-analysis.cpp

Index: clang/test/SemaCXX/warn-consumed-analysis.cpp
===
--- clang/test/SemaCXX/warn-consumed-analysis.cpp
+++ clang/test/SemaCXX/warn-consumed-analysis.cpp
@@ -953,12 +953,12 @@
 namespace std {
   void move();
   template
-  void move(T&&);
+  T &(T&);
 
   namespace __1 {
 void move();
 template
-void move(T&&);
+T &(T&);
   }
 }
 
@@ -971,7 +971,7 @@
   void test() {
 x.move();
 std::move();
-std::move(x);
+std::move(x); // expected-warning {{ignoring return value}}
 std::__1::move();
 std::__1::move(x);
   }
Index: clang/test/SemaCXX/unqualified-std-call.cpp
===
--- clang/test/SemaCXX/unqualified-std-call.cpp
+++ clang/test/SemaCXX/unqualified-std-call.cpp
@@ -1,17 +1,17 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s -Wno-unused-value
 
 namespace std {
 
 template 
 void dummy(T &&) {}
 template 
-void move(T &&) {}
+T &(T &) { return x; }
 template 
 void move(T &&, U &&) {}
 
 inline namespace __1 {
 template 
-void forward(T &) {}
+T (T ) { return x; }
 } // namespace __1
 
 struct foo {};
Index: clang/test/SemaCXX/unqualified-std-call-fixits.cpp
===
--- clang/test/SemaCXX/unqualified-std-call-fixits.cpp
+++ clang/test/SemaCXX/unqualified-std-call-fixits.cpp
@@ -6,9 +6,9 @@
 
 namespace std {
 
-void move(auto &) {}
+int &(auto &) { return a; }
 
-void forward(auto ) {}
+int &(auto ) { return a; }
 
 } // namespace std
 
@@ -16,8 +16,8 @@
 
 void f() {
   int i = 0;
-  move(i); // expected-warning {{unqualified call to std::move}}
-  // CHECK: {{^}}  std::
-  forward(i); // expected-warning {{unqualified call to std::forward}}
-  // CHECK: {{^}}  std::
+  (void)move(i); // expected-warning {{unqualified call to std::move}}
+  // CHECK: {{^}}  (void)std::move
+  (void)forward(i); // expected-warning {{unqualified call to std::forward}}
+  // CHECK: {{^}}  (void)std::forward
 }
Index: clang/test/SemaCXX/builtin-std-move.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtin-std-move.cpp
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -std=c++17 -verify %s
+// RUN: %clang_cc1 -std=c++17 -verify %s -DNO_CONSTEXPR
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+namespace std {
+#ifndef NO_CONSTEXPR
+#define CONSTEXPR constexpr
+#else
+#define CONSTEXPR
+#endif
+
+  template CONSTEXPR T &(T ) {
+static_assert(T::moveable, "instantiated move"); // expected-error {{no member named 'moveable' in 'B'}}
+ // expected-error@-1 {{no member named 'moveable' in 'C'}}
+return static_cast(x);
+  }
+
+  // Unrelated move functions are not the builtin.
+  template CONSTEXPR int move(T, T) { return 5; }
+
+  template struct ref { using type = T&; };
+  template struct ref { using type = T&&; };
+
+  template CONSTEXPR auto move_if_noexcept(T ) -> typename ref(x)))>::type {
+static_assert(T::moveable, "instantiated move_if_noexcept"); // expected-error {{no member named 'moveable' in 'B'}}
+return static_cast(x)))>::type>(x);
+  }
+
+  template struct remove_reference { using type = T; };
+  template struct remove_reference { using type = T; };
+  template struct remove_reference { using type = T; };
+
+  template CONSTEXPR T &(typename

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-15 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6577-6578
+// FIXME: This should also be in -Wc++23-compat once we have it.
+def warn_use_of_unaddressable_function : Warning<
+  "taking address of non-addressable standard library function">,
+  InGroup;

aaron.ballman wrote:
> aaron.ballman wrote:
> > Thank you for making this one on by default :-)
> Thoughts on this one?
Sorry, I was looking at your comments over email and didn't see the suggestion. 
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-15 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith updated this revision to Diff 423164.
rsmith marked 3 inline comments as done.
rsmith added a comment.

- Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

Files:
  clang/docs/CommandGuide/clang.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/Builtins.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Basic/Builtins.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Analysis/use-after-move.cpp
  clang/test/CodeGenCXX/builtin-std-move.cpp
  clang/test/CodeGenCXX/microsoft-abi-throw.cpp
  clang/test/SemaCXX/builtin-std-move-nobuiltin.cpp
  clang/test/SemaCXX/builtin-std-move.cpp
  clang/test/SemaCXX/unqualified-std-call-fixits.cpp
  clang/test/SemaCXX/unqualified-std-call.cpp
  clang/test/SemaCXX/warn-consumed-analysis.cpp

Index: clang/test/SemaCXX/warn-consumed-analysis.cpp
===
--- clang/test/SemaCXX/warn-consumed-analysis.cpp
+++ clang/test/SemaCXX/warn-consumed-analysis.cpp
@@ -953,12 +953,12 @@
 namespace std {
   void move();
   template
-  void move(T&&);
+  T &(T&);
 
   namespace __1 {
 void move();
 template
-void move(T&&);
+T &(T&);
   }
 }
 
@@ -971,7 +971,7 @@
   void test() {
 x.move();
 std::move();
-std::move(x);
+std::move(x); // expected-warning {{ignoring return value}}
 std::__1::move();
 std::__1::move(x);
   }
Index: clang/test/SemaCXX/unqualified-std-call.cpp
===
--- clang/test/SemaCXX/unqualified-std-call.cpp
+++ clang/test/SemaCXX/unqualified-std-call.cpp
@@ -1,17 +1,17 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s -Wno-unused-value
 
 namespace std {
 
 template 
 void dummy(T &&) {}
 template 
-void move(T &&) {}
+T &(T &) { return x; }
 template 
 void move(T &&, U &&) {}
 
 inline namespace __1 {
 template 
-void forward(T &) {}
+T (T ) { return x; }
 } // namespace __1
 
 struct foo {};
Index: clang/test/SemaCXX/unqualified-std-call-fixits.cpp
===
--- clang/test/SemaCXX/unqualified-std-call-fixits.cpp
+++ clang/test/SemaCXX/unqualified-std-call-fixits.cpp
@@ -6,9 +6,9 @@
 
 namespace std {
 
-void move(auto &) {}
+int &(auto &) { return a; }
 
-void forward(auto ) {}
+int &(auto ) { return a; }
 
 } // namespace std
 
@@ -16,8 +16,8 @@
 
 void f() {
   int i = 0;
-  move(i); // expected-warning {{unqualified call to std::move}}
-  // CHECK: {{^}}  std::
-  forward(i); // expected-warning {{unqualified call to std::forward}}
-  // CHECK: {{^}}  std::
+  (void)move(i); // expected-warning {{unqualified call to std::move}}
+  // CHECK: {{^}}  (void)std::move
+  (void)forward(i); // expected-warning {{unqualified call to std::forward}}
+  // CHECK: {{^}}  (void)std::forward
 }
Index: clang/test/SemaCXX/builtin-std-move.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtin-std-move.cpp
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -std=c++17 -verify %s
+// RUN: %clang_cc1 -std=c++17 -verify %s -DNO_CONSTEXPR
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+namespace std {
+#ifndef NO_CONSTEXPR
+#define CONSTEXPR constexpr
+#else
+#define CONSTEXPR
+#endif
+
+  template CONSTEXPR T &(T ) {
+static_assert(T::moveable, "instantiated move"); // expected-error {{no member named 'moveable' in 'B'}}
+ // expected-error@-1 {{no member named 'moveable' in 'C'}}
+return static_cast(x);
+  }
+
+  // Unrelated move functions are not the builtin.
+  template CONSTEXPR int move(T, T) { return 5; }
+
+  template struct ref { using type = T&; };
+  template struct ref { using type = T&&; };
+
+  template CONSTEXPR auto move_if_noexcept(T ) -> typename ref(x)))>::type {
+static_assert(T::moveable, "instantiated move_if_noexcept"); // expected-error {{no member named 'moveable' in 'B'}}
+return static_cast(x)))>::type>(x);
+  }
+
+  template struct remove_reference { using type = T; };
+  template struct remove_reference { using type = T; };
+  template struct remove_reference { using type = T; };
+
+  template CONSTEXPR T &(typename remove_reference::type ) {
+static_assert(T::moveable, "instantiated forward"); // expected-error {{no member named 'moveable' in 'B'}}
+//

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-15 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

In D123345#3453037 , @rnk wrote:

> Generally speaking, this sounds like a good idea to me. One time in 2019 I 
> used -ftime-trace+ClangBuildAnalyzer and it told me that std::unique_ptr was 
> the most expensive template 
>  because it is 
> instantiated so much. Those results don't even capture the -O0 object file 
> size impact.
>
> In D123345#3452933 , @rsmith wrote:
>
>> In D123345#3452496 , 
>> @aaron.ballman wrote:
>>
>>> Do you have ideas on how we can improve the debugging checkpoint behavior 
>>> (if at all)?
>>
>> I think we just live with it, like we do for other builtin functions. (There 
>> might be things we can do by emitting inlining info into the debug info. If 
>> we do that, we should presumably do it for all builtin lib functions.)
>
> Honestly, I don't think it's worth the debug info bytes to describe these 
> inlined call sites. Debug info isn't free.

+1 there - and also these operations/intrinsics produce no instructions, so far 
as I understand/know - so for now, LLVM's got to way to represent that anyway 
(there's some talk in DWARF about how to have multiple "states" for a single 
instruction location (so, eg, you could step in/out of an inlined function even 
though you stay at exactly the same instruction))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-15 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D123345#3452933 , @rsmith wrote:

> In D123345#3452496 , @aaron.ballman 
> wrote:
>
>> Do you have ideas on how we can improve the debugging checkpoint behavior 
>> (if at all)?
>
> I think we just live with it, like we do for other builtin functions. (There 
> might be things we can do by emitting inlining info into the debug info. If 
> we do that, we should presumably do it for all builtin lib functions.)

Okie dokie, so be it.

The changes LGTM aside from a previous question about diagnostic wording (feel 
free to accept the suggestion or not as you see fit). Thanks for working on 
this!




Comment at: clang/docs/ReleaseNotes.rst:121-124
+- Improved ``-O0`` code generation for calls to ``std::move``, 
``std::forward``,
+  and ``std::move_if_noexcept``. These are now treated as compiler builtins and
+  implemented directly, rather than instantiating the definition from the
+  standard library.

Probably worth moving down to the C++ language changes section because it's C++ 
specific.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6577-6578
+// FIXME: This should also be in -Wc++23-compat once we have it.
+def warn_use_of_unaddressable_function : Warning<
+  "taking address of non-addressable standard library function">,
+  InGroup;

aaron.ballman wrote:
> Thank you for making this one on by default :-)
Thoughts on this one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-14 Thread Reid Kleckner via Phabricator via cfe-commits 
rnk added a comment.

Generally speaking, this sounds like a good idea to me. One time in 2019 I used 
-ftime-trace+ClangBuildAnalyzer and it told me that std::unique_ptr was the 
most expensive template 
 because it is 
instantiated so much. Those results don't even capture the -O0 object file size 
impact.

In D123345#3452933 , @rsmith wrote:

> In D123345#3452496 , @aaron.ballman 
> wrote:
>
>> Do you have ideas on how we can improve the debugging checkpoint behavior 
>> (if at all)?
>
> I think we just live with it, like we do for other builtin functions. (There 
> might be things we can do by emitting inlining info into the debug info. If 
> we do that, we should presumably do it for all builtin lib functions.)

Honestly, I don't think it's worth the debug info bytes to describe these 
inlined call sites. Debug info isn't free.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-14 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

In D123345#3452496 , @aaron.ballman 
wrote:

> Do you have ideas on how we can improve the debugging checkpoint behavior (if 
> at all)?

I think we just live with it, like we do for other builtin functions. (There 
might be things we can do by emitting inlining info into the debug info. If we 
do that, we should presumably do it for all builtin lib functions.)




Comment at: clang/test/SemaCXX/builtin-std-move.cpp:12
+
+  template CONSTEXPR T &(T ) {
+static_assert(T::moveable, "instantiated move"); // expected-error {{no 
member named 'moveable' in 'B'}}

aaron.ballman wrote:
> rsmith wrote:
> > aaron.ballman wrote:
> > > Formatting looks like it's gone wonky in this file.
> > What are you referring to? The lines longer than 80 columns, or something 
> > else?
> > 
> > FWIW, long lines are common in our `test/` files. Substantially more than 
> > half of them go over 80 columns:
> > ```
> > $ rgrep '.\{81\}' clang/test  --files-with-match | wc -l
> > 12481
> > $ rgrep '.\{81\}' clang/test  --files-without-match | wc -l
> > 7194
> > ```
> Oh, the line length doesn't bother me (we smash all over that in tests, as 
> you've seen). It was more the extra indentation within a namespace -- we 
> don't usually indent like that and it caught me off guard. It's not a major 
> deal though (the same indentation happens in 
> clang/test/SemaCXX/builtin-std-move-nobuiltin.cpp).
Gotcha.

This is just what my editor / fingers happened to do, but now I'm consciously 
thinking about it, I see a bit of nuance here: I think it makes sense to not 
indent a namespace that spans a whole file, because the indent doesn't add 
anything for the reader and loses two columns of useful screen real estate. But 
when the namespace is covering a relatively small scope it seems useful to me 
to indent it to contrast the contents of the namespace against the surrounding 
code that's outside the namespace, in the same way it makes sense to indent a 
class definition to contrast the class scope against surrounding code.

It looks like indenting namespace bodies is pretty common in the clang test 
suite: of the 1917 files that contain a line matching /^namespace.*$/, 1171 of 
those files have a next line that is indented, which I think makes sense 
because namespaces in tests will tend to be short, and intended to semantically 
separate a portion of the test from the rest of the file. (By contrast, only 
~10% of the clang sources and headers have indented namespaces, and from a 
quick sampling it looks like they basically follow the same pattern: indented 
namespaces are mostly used for short scopes and unindented ones are mostly used 
for longer / whole-file scopes.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-14 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D123345#3446495 , @rsmith wrote:

> In D123345#3440935 , @aaron.ballman 
> wrote:
>
>> Any chance you'd be interested in submitting this patch to 
>> http://llvm-compile-time-tracker.com/ (instructions at 
>> http://llvm-compile-time-tracker.com/about.php) so we can have an idea of 
>> how compile times are impacted? (It's not critical, but having some more 
>> compile time performance measurements would be helpful to validate that this 
>> actually saves compile time as expected.
>
> Nice idea, done:
>
> - Instructions executed while compiling reduced by 0.3-0.6% 
> 
>  at `-O0` on C++ benchmarks kimwitu++ and tramp3d-v4. Changes in the noise on 
> other benchmarks and with optimizations enabled.
> - Code size reduced by 0.51% 
> 
>  at `-O0` on those same benchmarks. No change on any others.
> - Effect on compiler memory usage 
> 
>  appears to be below the noise floor.

Awesome, thank you for that information! Not a massive win, but definitely 
seems like an improvement worth having.

> In D123345#3442809 , @joerg wrote:
>
>> The patch contains at least one user visible change that would be quite 
>> surprising: it is no longer possible to intentionally set a break point on 
>> `std::move`.
>
> Yeah, I think it's reasonable to support `-fno-builtin-std-move` and the like 
> -- done. The intended model here is to treat these `std::` functions just 
> like how we treat the functions in the global namespace for which we provide 
> built-in semantics, so I think `-fno-builtin` and `-ffreestanding` and the 
> like should all treat these functions the same way they treat `malloc` and 
> `strlen` and such.

I can live with it. :-)

>> Thinking more about it, what about a slightly different implementation 
>> strategy? Provide a compiler built-in `__builtin_std_move`. If it is 
>> present, libc++ can alias it into `namespace std` using regular C++. Much of 
>> the name-matching and argument checking in various places disappears. The 
>> check to skip template instantiation can be done the same way. Over-all, it 
>> should be much less intrusive with the same performance benefits for an 
>> built-in-aware STL. It completely side-steps the question of ignoring the 
>> actual implementation `of std::move` in the source, because there is none.
>
> This is an interesting idea, but I don't think it would work out well. As a 
> superficial problem, C++ aliases (eg, `using` declarations) don't let you 
> change the name of a function. More fundamentally, I think this would end up 
> being a lot more complicated: we'd need to implement the rules for how to 
> deduce parameter types and transform them into return types ourselves, we'd 
> need to somehow support `std::forward`'s explicit template argument, and in 
> C++17 and before, it's valid to take the address of `std::move` and co, which 
> would require us to invent a function definition for them in IR generation. 
> This would also be a major deviation from how we treat all other standard 
> library functions for which we have built-in knowledge.

Ah drat, great point about not being able to change the name of the function 
and the extra complexity.

Do you have ideas on how we can improve the debugging checkpoint behavior (if 
at all)?




Comment at: clang/lib/Sema/SemaInit.cpp:8192-8195
+  // We might get back another placeholder expression if we resolved to a
+  // builtin.
+  if (!CurInit.isInvalid())
+CurInit = S.CheckPlaceholderExpr(CurInit.get());

rsmith wrote:
> aaron.ballman wrote:
> > Do we need to make this call every time we've called 
> > `FixOverloadedFunctionReference()`? I guess I'm not seeing the changes that 
> > necessitated this call (or the one below).
> We only need to do this in places that assume that the result doesn't have a 
> placeholder type. Specifically, `FixOverloadedFunctionReference` can now take 
> us from an expression whose type is the "overloaded function" builtin type to 
> an expression whose type is the "builtin function" builtin type, after 
> overload resolution picks a specific template specialization, which these 
> places were not prepared for (they thought they'd always get a real function 
> type).
> 
> In this case, we first build an initialization sequence (for example, for 
> `int &&(*p)(int&) = std::move;`, we'd build this sequence: (1) resolve 
> `std::move` to

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-12 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: clang/docs/CommandGuide/clang.rst:264
+
+.. option:: -fno-builtin-std-
+

xbolva00 wrote:
> -fno-builtin=xxx,yyy,zzz
> 
> wdyt?
Seems reasonable to me to treat `-fno-builtin-` and `-fno-builtin=` 
as equivalent, and I certainly prefer the latter form. I think that's somewhat 
orthogonal to this patch, though: I'm not introducing `-fno-builtin-blah` here, 
only documenting it. (It's a GCC flag originally.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-12 Thread Dávid Bolvanský via Phabricator via cfe-commits 
xbolva00 added inline comments.



Comment at: clang/docs/CommandGuide/clang.rst:264
+
+.. option:: -fno-builtin-std-
+

-fno-builtin=xxx,yyy,zzz

wdyt?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-12 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith updated this revision to Diff 422366.
rsmith added a comment.

- Switch to a different example with underscores.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

Files:
  clang/docs/CommandGuide/clang.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/Builtins.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Basic/Builtins.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Analysis/use-after-move.cpp
  clang/test/CodeGenCXX/builtin-std-move.cpp
  clang/test/CodeGenCXX/microsoft-abi-throw.cpp
  clang/test/SemaCXX/builtin-std-move-nobuiltin.cpp
  clang/test/SemaCXX/builtin-std-move.cpp
  clang/test/SemaCXX/unqualified-std-call-fixits.cpp
  clang/test/SemaCXX/unqualified-std-call.cpp
  clang/test/SemaCXX/warn-consumed-analysis.cpp

Index: clang/test/SemaCXX/warn-consumed-analysis.cpp
===
--- clang/test/SemaCXX/warn-consumed-analysis.cpp
+++ clang/test/SemaCXX/warn-consumed-analysis.cpp
@@ -953,12 +953,12 @@
 namespace std {
   void move();
   template
-  void move(T&&);
+  T &(T&);
 
   namespace __1 {
 void move();
 template
-void move(T&&);
+T &(T&);
   }
 }
 
@@ -971,7 +971,7 @@
   void test() {
 x.move();
 std::move();
-std::move(x);
+std::move(x); // expected-warning {{ignoring return value}}
 std::__1::move();
 std::__1::move(x);
   }
Index: clang/test/SemaCXX/unqualified-std-call.cpp
===
--- clang/test/SemaCXX/unqualified-std-call.cpp
+++ clang/test/SemaCXX/unqualified-std-call.cpp
@@ -1,17 +1,17 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s -Wno-unused-value
 
 namespace std {
 
 template 
 void dummy(T &&) {}
 template 
-void move(T &&) {}
+T &(T &) { return x; }
 template 
 void move(T &&, U &&) {}
 
 inline namespace __1 {
 template 
-void forward(T &) {}
+T (T ) { return x; }
 } // namespace __1
 
 struct foo {};
Index: clang/test/SemaCXX/unqualified-std-call-fixits.cpp
===
--- clang/test/SemaCXX/unqualified-std-call-fixits.cpp
+++ clang/test/SemaCXX/unqualified-std-call-fixits.cpp
@@ -6,9 +6,9 @@
 
 namespace std {
 
-void move(auto &) {}
+int &(auto &) { return a; }
 
-void forward(auto ) {}
+int &(auto ) { return a; }
 
 } // namespace std
 
@@ -16,8 +16,8 @@
 
 void f() {
   int i = 0;
-  move(i); // expected-warning {{unqualified call to std::move}}
-  // CHECK: {{^}}  std::
-  forward(i); // expected-warning {{unqualified call to std::forward}}
-  // CHECK: {{^}}  std::
+  (void)move(i); // expected-warning {{unqualified call to std::move}}
+  // CHECK: {{^}}  (void)std::move
+  (void)forward(i); // expected-warning {{unqualified call to std::forward}}
+  // CHECK: {{^}}  (void)std::forward
 }
Index: clang/test/SemaCXX/builtin-std-move.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtin-std-move.cpp
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -std=c++17 -verify %s
+// RUN: %clang_cc1 -std=c++17 -verify %s -DNO_CONSTEXPR
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+namespace std {
+#ifndef NO_CONSTEXPR
+#define CONSTEXPR constexpr
+#else
+#define CONSTEXPR
+#endif
+
+  template CONSTEXPR T &(T ) {
+static_assert(T::moveable, "instantiated move"); // expected-error {{no member named 'moveable' in 'B'}}
+ // expected-error@-1 {{no member named 'moveable' in 'C'}}
+return static_cast(x);
+  }
+
+  // Unrelated move functions are not the builtin.
+  template CONSTEXPR int move(T, T) { return 5; }
+
+  template struct ref { using type = T&; };
+  template struct ref { using type = T&&; };
+
+  template CONSTEXPR auto move_if_noexcept(T ) -> typename ref(x)))>::type {
+static_assert(T::moveable, "instantiated move_if_noexcept"); // expected-error {{no member named 'moveable' in 'B'}}
+return static_cast(x)))>::type>(x);
+  }
+
+  template struct remove_reference { using type = T; };
+  template struct remove_reference { using type = T; };
+  template struct remove_reference { using type = T; };
+
+  template CONSTEXPR T &(typename remove_reference::type ) {
+static_assert(T::moveable, "instantiated forward"); // expected-error {{no member named 'moveable' in 'B'}}
+// expected-error@-1 {{no member

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-12 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

In D123345#3440935 , @aaron.ballman 
wrote:

> Any chance you'd be interested in submitting this patch to 
> http://llvm-compile-time-tracker.com/ (instructions at 
> http://llvm-compile-time-tracker.com/about.php) so we can have an idea of how 
> compile times are impacted? (It's not critical, but having some more compile 
> time performance measurements would be helpful to validate that this actually 
> saves compile time as expected.

Nice idea, done:

- Instructions executed while compiling reduced by 0.3-0.6% 

 at `-O0` on C++ benchmarks kimwitu++ and tramp3d-v4. Changes in the noise on 
other benchmarks and with optimizations enabled.
- Code size reduced by 0.51% 

 at `-O0` on those same benchmarks. No change on any others.
- Effect on compiler memory usage 

 appears to be below the noise floor.

In D123345#3442809 , @joerg wrote:

> The patch contains at least one user visible change that would be quite 
> surprising: it is no longer possible to intentionally set a break point on 
> `std::move`.

Yeah, I think it's reasonable to support `-fno-builtin-std-move` and the like 
-- done. The intended model here is to treat these `std::` functions just like 
how we treat the functions in the global namespace for which we provide 
built-in semantics, so I think `-fno-builtin` and `-ffreestanding` and the like 
should all treat these functions the same way they treat `malloc` and `strlen` 
and such.

> Thinking more about it, what about a slightly different implementation 
> strategy? Provide a compiler built-in `__builtin_std_move`. If it is present, 
> libc++ can alias it into `namespace std` using regular C++. Much of the 
> name-matching and argument checking in various places disappears. The check 
> to skip template instantiation can be done the same way. Over-all, it should 
> be much less intrusive with the same performance benefits for an 
> built-in-aware STL. It completely side-steps the question of ignoring the 
> actual implementation `of std::move` in the source, because there is none.

This is an interesting idea, but I don't think it would work out well. As a 
superficial problem, C++ aliases (eg, `using` declarations) don't let you 
change the name of a function. More fundamentally, I think this would end up 
being a lot more complicated: we'd need to implement the rules for how to 
deduce parameter types and transform them into return types ourselves, we'd 
need to somehow support `std::forward`'s explicit template argument, and in 
C++17 and before, it's valid to take the address of `std::move` and co, which 
would require us to invent a function definition for them in IR generation. 
This would also be a major deviation from how we treat all other standard 
library functions for which we have built-in knowledge.




Comment at: clang/lib/Sema/SemaExpr.cpp:20282
+  if (Context.BuiltinInfo.isInStdNamespace(BuiltinID)) {
+// Any use of these other than a direct call is ill-formed as of C++20,
+// because they are not addressable functions. In earlier language

aaron.ballman wrote:
> I wonder how often these get passed around as function objects... e.g.,
> ```
> template 
> void func(Fn &);
> 
> int main() {
>   func(std::move);
> }
> ```
> (I don't see evidence that this is a common code pattern, but C++ surprises 
> me often enough that I wonder if this will cause issues.)
I expect people will tell us if it this fires a lot... if it does, we can 
consider making the C++20 error a `DefaultError` `ExtWarn`.



Comment at: clang/lib/Sema/SemaInit.cpp:8192-8195
+  // We might get back another placeholder expression if we resolved to a
+  // builtin.
+  if (!CurInit.isInvalid())
+CurInit = S.CheckPlaceholderExpr(CurInit.get());

aaron.ballman wrote:
> Do we need to make this call every time we've called 
> `FixOverloadedFunctionReference()`? I guess I'm not seeing the changes that 
> necessitated this call (or the one below).
We only need to do this in places that assume that the result doesn't have a 
placeholder type. Specifically, `FixOverloadedFunctionReference` can now take 
us from an expression whose type is the "overloaded function" builtin type to 
an expression whose type is the "builtin function" builtin type, after overload 
resolution picks a specific template specialization, which these places were 
not prepared for (they thought

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-12 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith updated this revision to Diff 422311.
rsmith added a comment.

- Document `-fno-builtin-std-foo`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

Files:
  clang/docs/CommandGuide/clang.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/Builtins.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Basic/Builtins.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Analysis/use-after-move.cpp
  clang/test/CodeGenCXX/builtin-std-move.cpp
  clang/test/CodeGenCXX/microsoft-abi-throw.cpp
  clang/test/SemaCXX/builtin-std-move-nobuiltin.cpp
  clang/test/SemaCXX/builtin-std-move.cpp
  clang/test/SemaCXX/unqualified-std-call-fixits.cpp
  clang/test/SemaCXX/unqualified-std-call.cpp
  clang/test/SemaCXX/warn-consumed-analysis.cpp

Index: clang/test/SemaCXX/warn-consumed-analysis.cpp
===
--- clang/test/SemaCXX/warn-consumed-analysis.cpp
+++ clang/test/SemaCXX/warn-consumed-analysis.cpp
@@ -953,12 +953,12 @@
 namespace std {
   void move();
   template
-  void move(T&&);
+  T &(T&);
 
   namespace __1 {
 void move();
 template
-void move(T&&);
+T &(T&);
   }
 }
 
@@ -971,7 +971,7 @@
   void test() {
 x.move();
 std::move();
-std::move(x);
+std::move(x); // expected-warning {{ignoring return value}}
 std::__1::move();
 std::__1::move(x);
   }
Index: clang/test/SemaCXX/unqualified-std-call.cpp
===
--- clang/test/SemaCXX/unqualified-std-call.cpp
+++ clang/test/SemaCXX/unqualified-std-call.cpp
@@ -1,17 +1,17 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s -Wno-unused-value
 
 namespace std {
 
 template 
 void dummy(T &&) {}
 template 
-void move(T &&) {}
+T &(T &) { return x; }
 template 
 void move(T &&, U &&) {}
 
 inline namespace __1 {
 template 
-void forward(T &) {}
+T (T ) { return x; }
 } // namespace __1
 
 struct foo {};
Index: clang/test/SemaCXX/unqualified-std-call-fixits.cpp
===
--- clang/test/SemaCXX/unqualified-std-call-fixits.cpp
+++ clang/test/SemaCXX/unqualified-std-call-fixits.cpp
@@ -6,9 +6,9 @@
 
 namespace std {
 
-void move(auto &) {}
+int &(auto &) { return a; }
 
-void forward(auto ) {}
+int &(auto ) { return a; }
 
 } // namespace std
 
@@ -16,8 +16,8 @@
 
 void f() {
   int i = 0;
-  move(i); // expected-warning {{unqualified call to std::move}}
-  // CHECK: {{^}}  std::
-  forward(i); // expected-warning {{unqualified call to std::forward}}
-  // CHECK: {{^}}  std::
+  (void)move(i); // expected-warning {{unqualified call to std::move}}
+  // CHECK: {{^}}  (void)std::move
+  (void)forward(i); // expected-warning {{unqualified call to std::forward}}
+  // CHECK: {{^}}  (void)std::forward
 }
Index: clang/test/SemaCXX/builtin-std-move.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtin-std-move.cpp
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -std=c++17 -verify %s
+// RUN: %clang_cc1 -std=c++17 -verify %s -DNO_CONSTEXPR
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+namespace std {
+#ifndef NO_CONSTEXPR
+#define CONSTEXPR constexpr
+#else
+#define CONSTEXPR
+#endif
+
+  template CONSTEXPR T &(T ) {
+static_assert(T::moveable, "instantiated move"); // expected-error {{no member named 'moveable' in 'B'}}
+ // expected-error@-1 {{no member named 'moveable' in 'C'}}
+return static_cast(x);
+  }
+
+  // Unrelated move functions are not the builtin.
+  template CONSTEXPR int move(T, T) { return 5; }
+
+  template struct ref { using type = T&; };
+  template struct ref { using type = T&&; };
+
+  template CONSTEXPR auto move_if_noexcept(T ) -> typename ref(x)))>::type {
+static_assert(T::moveable, "instantiated move_if_noexcept"); // expected-error {{no member named 'moveable' in 'B'}}
+return static_cast(x)))>::type>(x);
+  }
+
+  template struct remove_reference { using type = T; };
+  template struct remove_reference { using type = T; };
+  template struct remove_reference { using type = T; };
+
+  template CONSTEXPR T &(typename remove_reference::type ) {
+static_assert(T::moveable, "instantiated forward"); // expected-error {{no member named 'moveable' in 'B'}}
+// expected-error@-1 {{no member named 'moveable'

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-12 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith updated this revision to Diff 422305.
rsmith added a comment.

- Add support for -fno-builtin, -ffreestanding, -fno-builtin-std-move.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/Builtins.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Basic/Builtins.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Analysis/use-after-move.cpp
  clang/test/CodeGenCXX/builtin-std-move.cpp
  clang/test/CodeGenCXX/microsoft-abi-throw.cpp
  clang/test/SemaCXX/builtin-std-move-nobuiltin.cpp
  clang/test/SemaCXX/builtin-std-move.cpp
  clang/test/SemaCXX/unqualified-std-call-fixits.cpp
  clang/test/SemaCXX/unqualified-std-call.cpp
  clang/test/SemaCXX/warn-consumed-analysis.cpp

Index: clang/test/SemaCXX/warn-consumed-analysis.cpp
===
--- clang/test/SemaCXX/warn-consumed-analysis.cpp
+++ clang/test/SemaCXX/warn-consumed-analysis.cpp
@@ -953,12 +953,12 @@
 namespace std {
   void move();
   template
-  void move(T&&);
+  T &(T&);
 
   namespace __1 {
 void move();
 template
-void move(T&&);
+T &(T&);
   }
 }
 
@@ -971,7 +971,7 @@
   void test() {
 x.move();
 std::move();
-std::move(x);
+std::move(x); // expected-warning {{ignoring return value}}
 std::__1::move();
 std::__1::move(x);
   }
Index: clang/test/SemaCXX/unqualified-std-call.cpp
===
--- clang/test/SemaCXX/unqualified-std-call.cpp
+++ clang/test/SemaCXX/unqualified-std-call.cpp
@@ -1,17 +1,17 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s -Wno-unused-value
 
 namespace std {
 
 template 
 void dummy(T &&) {}
 template 
-void move(T &&) {}
+T &(T &) { return x; }
 template 
 void move(T &&, U &&) {}
 
 inline namespace __1 {
 template 
-void forward(T &) {}
+T (T ) { return x; }
 } // namespace __1
 
 struct foo {};
Index: clang/test/SemaCXX/unqualified-std-call-fixits.cpp
===
--- clang/test/SemaCXX/unqualified-std-call-fixits.cpp
+++ clang/test/SemaCXX/unqualified-std-call-fixits.cpp
@@ -6,9 +6,9 @@
 
 namespace std {
 
-void move(auto &) {}
+int &(auto &) { return a; }
 
-void forward(auto ) {}
+int &(auto ) { return a; }
 
 } // namespace std
 
@@ -16,8 +16,8 @@
 
 void f() {
   int i = 0;
-  move(i); // expected-warning {{unqualified call to std::move}}
-  // CHECK: {{^}}  std::
-  forward(i); // expected-warning {{unqualified call to std::forward}}
-  // CHECK: {{^}}  std::
+  (void)move(i); // expected-warning {{unqualified call to std::move}}
+  // CHECK: {{^}}  (void)std::move
+  (void)forward(i); // expected-warning {{unqualified call to std::forward}}
+  // CHECK: {{^}}  (void)std::forward
 }
Index: clang/test/SemaCXX/builtin-std-move.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtin-std-move.cpp
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -std=c++17 -verify %s
+// RUN: %clang_cc1 -std=c++17 -verify %s -DNO_CONSTEXPR
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+namespace std {
+#ifndef NO_CONSTEXPR
+#define CONSTEXPR constexpr
+#else
+#define CONSTEXPR
+#endif
+
+  template CONSTEXPR T &(T ) {
+static_assert(T::moveable, "instantiated move"); // expected-error {{no member named 'moveable' in 'B'}}
+ // expected-error@-1 {{no member named 'moveable' in 'C'}}
+return static_cast(x);
+  }
+
+  // Unrelated move functions are not the builtin.
+  template CONSTEXPR int move(T, T) { return 5; }
+
+  template struct ref { using type = T&; };
+  template struct ref { using type = T&&; };
+
+  template CONSTEXPR auto move_if_noexcept(T ) -> typename ref(x)))>::type {
+static_assert(T::moveable, "instantiated move_if_noexcept"); // expected-error {{no member named 'moveable' in 'B'}}
+return static_cast(x)))>::type>(x);
+  }
+
+  template struct remove_reference { using type = T; };
+  template struct remove_reference { using type = T; };
+  template struct remove_reference { using type = T; };
+
+  template CONSTEXPR T &(typename remove_reference::type ) {
+static_assert(T::moveable, "instantiated forward"); // expected-error {{no member named 'moveable' in 'B'}}
+// expected-error@-1 {{no member named 'moveable'

Re: [PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-12 Thread Arthur O'Dwyer via cfe-commits 
On Mon, Apr 11, 2022 at 12:14 PM Aaron Ballman via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> On Mon, Apr 11, 2022 at 10:50 AM Joerg Sonnenberger via Phabricator <
> revi...@reviews.llvm.org> wrote:
> >
> > joerg added a comment.
> >
> > The patch contains at least one user visible change that would be quite
> surprising: it is no longer possible to intentionally set a break point on
> `std::move`.
>
> Thank you, that is a really good point. It would be a surprise to set
> a breakpoint on the code I can see in debug mode (-O0 -g) and never
> hit the breakpoint.
>

The same is true of `std::strlen`: if it is builtin'ed, then you don't get
a call instruction. This is perfectly expected and natural.
https://godbolt.org/z/jYzTM4a7c
Passing -fno-builtin successfully disables the optimization for
std::strlen. It should also disable the proposed optimization for std::move.

Contra Aaron, I do not see *any difference at all* between what happens
today for std::strlen and what's proposed to happen tomorrow for
std::move.  I don't think std::move is somehow "more advanced" or "more
STL'ish" or "more C++'ish" than std::strlen. They're both part of the
standard library, and the compiler in hosted mode is allowed to assume it
knows what they do — but in freestanding mode (when -fno-builtin is passed)
the compiler should *not* assume it knows what they do.


> Thinking more about it, what about a slightly different implementation
> strategy? Provide a compiler built-in `__builtin_std_move`. If it is
> present, libc++ can alias it into `namespace std` using regular C++.


FWIW, Joerg, I don't understand this suggestion. By what means can libc++
"alias [a compiler builtin] into namespace std"? The only thing I have
imagined so far is something like
inline auto move(auto&& t) { return __builtin_std_move(t); }
but that would defeat the whole purpose of adding the builtin. The point of
adding the builtin is to eliminate the cost of
instantiating+codegenning+inlining, a whole new function for every call.
In C you could get away with
#define move(x) __builtin_std_move(x)
but that's a non-starter in C++, because namespaces.

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

Re: [PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-11 Thread Aaron Ballman via cfe-commits 
On Mon, Apr 11, 2022 at 10:50 AM Joerg Sonnenberger via Phabricator
 wrote:
>
> joerg added a comment.
>
> The patch contains at least one user visible change that would be quite 
> surprising: it is no longer possible to intentionally set a break point on 
> `std::move`.

Thank you, that is a really good point. It would be a surprise to set
a breakpoint on the code I can see in debug mode (-O0 -g) and never
hit the breakpoint.

> Thinking more about it, what about a slightly different implementation 
> strategy? Provide a compiler built-in `__builtin_std_move`. If it is present, 
> libc++ can alias it into `namespace std` using regular C++. Much of the 
> name-matching and argument checking in various places disappears. The check 
> to skip template instantiation can be done the same way. Over-all, it should 
> be much less intrusive with the same performance benefits for an 
> built-in-aware STL. It completely side-steps the question of ignoring the 
> actual implementation `of std::move` in the source, because there is none.

This is a pretty neat idea that seems worth exploring; it certainly
makes me more comfortable than adding comment line options that
suggest users can reliably modify things in namespace std. Thanks for
the suggestion!

~Aaron

>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D123345/new/
>
> https://reviews.llvm.org/D123345
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-11 Thread Joerg Sonnenberger via Phabricator via cfe-commits 
joerg added a comment.

The patch contains at least one user visible change that would be quite 
surprising: it is no longer possible to intentionally set a break point on 
`std::move`.

Thinking more about it, what about a slightly different implementation 
strategy? Provide a compiler built-in `__builtin_std_move`. If it is present, 
libc++ can alias it into `namespace std` using regular C++. Much of the 
name-matching and argument checking in various places disappears. The check to 
skip template instantiation can be done the same way. Over-all, it should be 
much less intrusive with the same performance benefits for an built-in-aware 
STL. It completely side-steps the question of ignoring the actual 
implementation `of std::move` in the source, because there is none.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

Re: [PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-11 Thread Aaron Ballman via cfe-commits 
Somehow this entire thread seems to have dropped the mailing lists and
the review in question, so top-posting to ensure the discussion at
least makes it to the mailing lists.

~Aaron

On Mon, Apr 11, 2022 at 9:10 AM Aaron Ballman  wrote:
>
> On Mon, Apr 11, 2022 at 8:25 AM Joerg Sonnenberger  wrote:
> >
> > Am Mon, Apr 11, 2022 at 07:42:15AM -0400 schrieb Aaron Ballman:
> > > On Mon, Apr 11, 2022 at 7:21 AM Joerg Sonnenberger  wrote:
> > > > You don't see the value of being able to instrument std::move? I don't
> > > > even mean in the sense of the operator new overload. Being able to patch
> > > > libc++ to add a tracking feature seems like an entirely sensible thing
> > > > to support.
> > >
> > > I'd want to hear from STL maintainers whether *they* think it's
> > > reasonable. Every STL maintainer I've ever talked to seems to think
> > > letting users add their own symbols to 'std' is highly problematic, so
> > > I'm not certain that it's a good idea to make that easier and give the
> > > appearance that we bless such a thing as a toolchain. (This goes
> > > beyond libc++ given that Clang also supports other STL implementations
> > > like libstdc++.)
> >
> > Again, this is not about adding symbols to 'std'. You make it impossible
> > to instrument std::move and friends in the name of performance for
> > unoptimized code. Without any mechanism to disable it. If I want to
> > create my own fork of libc++ or libstdc++ or whatever to have such
> > functionality, it should be possible.
>
> Should it? The STL is part of the implementation in C++, so I think
> it's perfectly reasonable to assume the burden is on you to fork the
> compiler as well if you have to.
>
> The root of my concern is: I don't want to expose an option that gives
> the impression it's okay to replace STL functions (especially in *all
> language modes*) given that it is explicitly UB and WG21 has a
> standing document on the subject (https://wg21.link/sd-8). That's not
> an interface I think we should support *unless STL maintainers
> explicitly agree that this is a reasonable policy in general*. (If you
> wanted this to be a CC1 option, that's a bit different to me because
> then it's not giving the same "this is normal, go right ahead and do
> it" feeling with it. My concerns are only with a user-facing option
> and are specifically because the situation you want to support is
> explicitly undefined behavior.)
>
> > I'm not even saying that the
> > semantic analysis here should detect that the std::move implementation
> > is actually std::move, which would side step this question as well.
> > We have a long history of optimization-by-symbol-name breaking
> > reasonable real world code. That's where -fno-builtin-* came from and
> > checks e.g. to allow implementing calloc in C without the compiler
> > turning the code of malloc+memset into a recursive call. I don't see
> > this change here being any different to the nature of those checks.
>
> I see that as different -- those interfaces are in the global
> namespace and thus available for the user to reuse in a freestanding
> environment with different semantics. Giving users a knob to support
> that situation seems eminently reasonable because it's not UB to
> replace those functions.
>
> That said, I'm happy to leave the decision to Richard as code owner.
> If he thinks this is worth maintaining a flag for, I can hold my nose.
>
> ~Aaron
>
> >
> > Joerg
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-10 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

In D123345#3441262 , @joerg wrote:

> As is, I think this conflicts with `-ffreestanding` assumptions or at the 
> very least the spirit.

Why? These functions are in `` which is not required in freestanding, 
but implementations are allowed to support more anyway 
(http://eel.is/c++draft/compliance#2). As the codegen doesn't emit a call to a 
library function but is purely using language facilities to reimplement the 
functionality, these don't seem to be in conflict with freestanding to me. If 
you could expound on what problems you see, that'd be helpful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-09 Thread Joerg Sonnenberger via Phabricator via cfe-commits 
joerg added a comment.

As is, I think this conflicts with `-ffreestanding` assumptions or at the very 
least the spirit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-09 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added a comment.

Thank you for this, it's a great improvement! I think it's generally correct, 
but I did have some minor questions.

> Most of these improvements are very small, but I measured a 3% decrease in 
> -O0 object file size for a simple C++ source file using the standard library 
> after this change.

Any chance you'd be interested in submitting this patch to 
http://llvm-compile-time-tracker.com/ (instructions at 
http://llvm-compile-time-tracker.com/about.php) so we can have an idea of how 
compile times are impacted? (It's not critical, but having some more compile 
time performance measurements would be helpful to validate that this actually 
saves compile time as expected.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6577-6578
+// FIXME: This should also be in -Wc++23-compat once we have it.
+def warn_use_of_unaddressable_function : Warning<
+  "taking address of non-addressable standard library function">,
+  InGroup;

Thank you for making this one on by default :-)



Comment at: clang/lib/Sema/SemaExpr.cpp:20282
+  if (Context.BuiltinInfo.isInStdNamespace(BuiltinID)) {
+// Any use of these other than a direct call is ill-formed as of C++20,
+// because they are not addressable functions. In earlier language

I wonder how often these get passed around as function objects... e.g.,
```
template 
void func(Fn &);

int main() {
  func(std::move);
}
```
(I don't see evidence that this is a common code pattern, but C++ surprises me 
often enough that I wonder if this will cause issues.)



Comment at: clang/lib/Sema/SemaInit.cpp:8192-8195
+  // We might get back another placeholder expression if we resolved to a
+  // builtin.
+  if (!CurInit.isInvalid())
+CurInit = S.CheckPlaceholderExpr(CurInit.get());

Do we need to make this call every time we've called 
`FixOverloadedFunctionReference()`? I guess I'm not seeing the changes that 
necessitated this call (or the one below).



Comment at: clang/test/SemaCXX/builtin-std-move.cpp:12
+
+  template CONSTEXPR T &(T ) {
+static_assert(T::moveable, "instantiated move"); // expected-error {{no 
member named 'moveable' in 'B'}}

Formatting looks like it's gone wonky in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345

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

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

2022-04-07 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith created this revision.
rsmith added reviewers: aaron.ballman, rnk.
Herald added a project: All.
rsmith requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We still require these functions to be declared before they can be used,
but don't instantiate their definitions unless their addresses are
taken. Instead, code generation, constant evaluation, and static
analysis are given direct knowledge of their effect.

This change aims to reduce various costs associated with these functions

- per-instantiation memory costs, compile time and memory costs due to

creating out-of-line copies and inlining them, code size at -O0, and so
on -- so that they are not substantially more expensive than a cast.
Most of these improvements are very small, but I measured a 3% decrease
in -O0 object file size for a simple C++ source file using the standard
library after this change.

We now automatically infer the `const` and `nothrow` attributes on these
now-builtin functions, in particular meaning that we get a warning for
an unused call to one of these functions.

In C++20 onwards, we disallow taking the addresses of these functions,
per the C++20 "addressable function" rule. In earlier language modes, a
compatibility warning is produced but the address can still be taken.

The same infrastructure is extended to the existing MSVC builtin
`__GetExceptionInfo`, which is now only recognized in namespace `std`
like it always should have been.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123345

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/Builtins.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Basic/Builtins.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/Analysis/use-after-move.cpp
  clang/test/CodeGenCXX/builtin-std-move.cpp
  clang/test/CodeGenCXX/microsoft-abi-throw.cpp
  clang/test/SemaCXX/builtin-std-move.cpp
  clang/test/SemaCXX/unqualified-std-call-fixits.cpp
  clang/test/SemaCXX/unqualified-std-call.cpp
  clang/test/SemaCXX/warn-consumed-analysis.cpp

Index: clang/test/SemaCXX/warn-consumed-analysis.cpp
===
--- clang/test/SemaCXX/warn-consumed-analysis.cpp
+++ clang/test/SemaCXX/warn-consumed-analysis.cpp
@@ -953,12 +953,12 @@
 namespace std {
   void move();
   template
-  void move(T&&);
+  T &(T&);
 
   namespace __1 {
 void move();
 template
-void move(T&&);
+T &(T&);
   }
 }
 
@@ -971,7 +971,7 @@
   void test() {
 x.move();
 std::move();
-std::move(x);
+std::move(x); // expected-warning {{ignoring return value}}
 std::__1::move();
 std::__1::move(x);
   }
Index: clang/test/SemaCXX/unqualified-std-call.cpp
===
--- clang/test/SemaCXX/unqualified-std-call.cpp
+++ clang/test/SemaCXX/unqualified-std-call.cpp
@@ -1,17 +1,17 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -std=c++11 %s -Wno-unused-value
 
 namespace std {
 
 template 
 void dummy(T &&) {}
 template 
-void move(T &&) {}
+T &(T &) { return x; }
 template 
 void move(T &&, U &&) {}
 
 inline namespace __1 {
 template 
-void forward(T &) {}
+T (T ) { return x; }
 } // namespace __1
 
 struct foo {};
Index: clang/test/SemaCXX/unqualified-std-call-fixits.cpp
===
--- clang/test/SemaCXX/unqualified-std-call-fixits.cpp
+++ clang/test/SemaCXX/unqualified-std-call-fixits.cpp
@@ -6,9 +6,9 @@
 
 namespace std {
 
-void move(auto &) {}
+int &(auto &) { return a; }
 
-void forward(auto ) {}
+int &(auto ) { return a; }
 
 } // namespace std
 
@@ -16,8 +16,8 @@
 
 void f() {
   int i = 0;
-  move(i); // expected-warning {{unqualified call to std::move}}
-  // CHECK: {{^}}  std::
-  forward(i); // expected-warning {{unqualified call to std::forward}}
-  // CHECK: {{^}}  std::
+  (void)move(i); // expected-warning {{unqualified call to std::move}}
+  // CHECK: {{^}}  (void)std::move
+  (void)forward(i); // expected-warning {{unqualified call to std::forward}}
+  // CHECK: {{^}}  (void)std::forward
 }
Index: clang/test/SemaCXX/builtin-std-move.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtin-std-move.cpp
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -std=c++17 -verify %s
+// RUN: %clang_cc1 -std=c++17 -verify %s -DNO_CONSTEXPR
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+namespace std {
+#ifndef NO_CONSTEXPR
+#define