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

Thu Apr 14 11:51:11 PDT 2022

aaron.ballman added a comment.

In D123345#3446495 <https://reviews.llvm.org/D123345#3446495>, @rsmith wrote:

> In D123345#3440935 <https://reviews.llvm.org/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% <http://llvm-compile-time-tracker.com/compare.php?from=f1b0a4fc540f986372f09768c325d505b75b414d&to=c323120bcc8a8b060f7a14d96d93a4c4a5530617&stat=instructions> 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% <http://llvm-compile-time-tracker.com/compare.php?from=f1b0a4fc540f986372f09768c325d505b75b414d&to=c323120bcc8a8b060f7a14d96d93a4c4a5530617&stat=size-text> at `-O0` on those same benchmarks. No change on any others.
> - Effect on compiler memory usage <http://llvm-compile-time-tracker.com/compare.php?from=f1b0a4fc540f986372f09768c325d505b75b414d&to=c323120bcc8a8b060f7a14d96d93a4c4a5530617&stat=max-rss> 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 <https://reviews.llvm.org/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 `std::move<int>`, then (2) decay `std::move<int>` to a function pointer), and then we apply it. If step (1) leaves us with a builtin function, we end up decaying *that* to a pointer, producing `<builtin-function-type>*`, which is an invalid type. The other case is similar: we first build an implicit conversion sequence then apply it and assume it will make sense. Other places that call `FixOverloadedFunctionReference` don't assume that they can blindly perform operations on its result, so they don't need this handling.
> 
> This never happened before because we never before had a builtin function that both requires overload resolution and uses a placeholder type as the type of a reference to the function. We do the latter here so that we can detect uses of the function that aren't calls, in order to trigger a real instantiation of the body in the rare cases where they're needed.
Ah, thank you for that explanation, that helps a bunch!

================
Comment at: clang/test/SemaCXX/builtin-std-move.cpp:12
+
+  template<typename T> CONSTEXPR T &&move(T &x) {
+    static_assert(T::moveable, "instantiated move"); // expected-error {{no member named 'moveable' in 'B'}}
----------------
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).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123345