rsmith added a comment. 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. 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. > 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 <typename Fn> > void func(Fn &&Call); > > int main() { > func(std::move<int>); > } > ``` > (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 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. ================ 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'}} ---------------- 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 ``` 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
