-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[libc++] Add basic constant folding for std::format #107197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
philnik777
commented
Sep 4, 2024
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) Changes
Full diff: https://github.com/llvm/llvm-project/pull/107197.diff 2 Files Affected:
diff --git a/libcxx/include/__format/format_functions.h b/libcxx/include/__format/format_functions.h
index 1518ab5768d243..dbb0b7bfcb12b4 100644
--- a/libcxx/include/__format/format_functions.h
+++ b/libcxx/include/__format/format_functions.h
@@ -11,6 +11,7 @@
#define _LIBCPP___FORMAT_FORMAT_FUNCTIONS
#include <__algorithm/clamp.h>
+#include <__algorithm/ranges_find_first_of.h>
#include <__concepts/convertible_to.h>
#include <__concepts/same_as.h>
#include <__config>
@@ -448,13 +449,44 @@ format_to(_OutIt __out_it, wformat_string<_Args...> __fmt, _Args&&... __args) {
}
# endif
+template <class _CharT>
+[[nodiscard]] optional<basic_string<_CharT>> __try_constant_folding(
+ basic_string_view<_CharT> __fmt,
+ basic_format_args<basic_format_context<back_insert_iterator<__format::__output_buffer<_CharT>>, _CharT>> __args) {
+ return nullopt;
+ if (bool __is_identity = std::ranges::find_first_of(__fmt, array{'{', '}'}) == __fmt.end();
+ __builtin_constant_p(__is_identity) && __is_identity)
+ return basic_string<_CharT>{__fmt};
+
+ if (auto __only_first_arg = __fmt == _LIBCPP_STATICALLY_WIDEN(_CharT, "{}");
+ __builtin_constant_p(__only_first_arg) && __only_first_arg) {
+ if (auto __arg = __args.get(0); __builtin_constant_p(__arg.__type_)) {
+ return std::__visit_format_arg(
+ []<class _Tp>(_Tp&& __argument) -> optional<basic_string<_CharT>> {
+ if constexpr (is_same_v<remove_cvref_t<_Tp>, basic_string_view<_CharT>>) {
+ return basic_string<_CharT>{__argument};
+ } else {
+ return nullopt;
+ }
+ },
+ __arg);
+ }
+ }
+
+ return nullopt;
+}
+
// TODO FMT This needs to be a template or std::to_chars(floating-point) availability markup
// fires too eagerly, see http://llvm.org/PR61563.
template <class = void>
[[nodiscard]] _LIBCPP_ALWAYS_INLINE inline _LIBCPP_HIDE_FROM_ABI string vformat(string_view __fmt, format_args __args) {
- string __res;
- std::vformat_to(std::back_inserter(__res), __fmt, __args);
- return __res;
+ return std::__try_constant_folding(__fmt, __args)
+ .or_else([&]() -> optional<string> {
+ string __res;
+ std::vformat_to(std::back_inserter(__res), __fmt, __args);
+ return __res;
+ })
+ .value();
}
# ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
@@ -463,9 +495,13 @@ template <class = void>
template <class = void>
[[nodiscard]] _LIBCPP_ALWAYS_INLINE inline _LIBCPP_HIDE_FROM_ABI wstring
vformat(wstring_view __fmt, wformat_args __args) {
- wstring __res;
- std::vformat_to(std::back_inserter(__res), __fmt, __args);
- return __res;
+ return std::__try_constant_folding(__fmt, __args)
+ .or_else([&]() -> optional<wstring> {
+ wstring __res;
+ std::vformat_to(std::back_inserter(__res), __fmt, __args);
+ return __res;
+ })
+ .value();
}
# endif
diff --git a/libcxx/test/benchmarks/format.bench.cpp b/libcxx/test/benchmarks/format.bench.cpp
index 17eaccb18ee1c4..5632aa5b9810f3 100644
--- a/libcxx/test/benchmarks/format.bench.cpp
+++ b/libcxx/test/benchmarks/format.bench.cpp
@@ -28,4 +28,13 @@ static void BM_format_string(benchmark::State& state) {
BENCHMARK(BM_format_string<char>)->RangeMultiplier(2)->Range(1, 1 << 20);
BENCHMARK(BM_format_string<wchar_t>)->RangeMultiplier(2)->Range(1, 1 << 20);
+template <class CharT>
+static void BM_string_without_formatting(benchmark::State& state) {
+ for (auto _ : state) {
+ benchmark::DoNotOptimize(std::format(CSTR("Hello, World!")));
+ }
+}
+BENCHMARK(BM_string_without_formatting<char>);
+BENCHMARK(BM_string_without_formatting<wchar_t>);
+
BENCHMARK_MAIN();
|
44e0a17
to
3c4cb14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance I think this makes sense. I'm unsure how much this is actually buying us cause this pattern might be really really infrequent. However as @philnik777 pointed out just now this might be common when format
is used through std::print
.
One general thing I'd like to mention is that we should be mindful of making our implementations more heavyweight for these kinds of optimizations. I think they are great and most of the time worth it, but we should keep in mind that we're adding more dependencies and more "code" for the compiler to churn through. Not a problem with this patch, but we should keep this in our heads as we work on these optimizations to make sure we don't lose sight of the big picture.
3c4cb14
to
657a6cb
Compare
657a6cb
to
9be4e43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I seem to have missed this patch before,
Thanks for working on this!
I have a hard time to review the patch
- the patch does not describe what it does, there are only benchmarks
- the code has no comment.
Did you benchmark what to overhead for the types are?
basic_format_args<basic_format_context<back_insert_iterator<__format::__output_buffer<_CharT>>, _CharT>> __args) { | ||
// [[gnu::pure]] is added to ensure that the compiler always knows that this call can be eliminated. | ||
if (bool __is_identity = | ||
[&] [[__gnu__::__pure__]] { return std::ranges::find_first_of(__fmt, array{'{', '}'}) == __fmt.end(); }(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why testing against '{'
instead of _Chart('{')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for one over the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In format we typically do the latter, so it reads very odd to do things differently here. I'm even a bit surprised this compiles since you want to compare wchar_t
against char
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you benchmark what to overhead for the types are?
I'm not sure what you're trying to ask.
basic_format_args<basic_format_context<back_insert_iterator<__format::__output_buffer<_CharT>>, _CharT>> __args) { | ||
// [[gnu::pure]] is added to ensure that the compiler always knows that this call can be eliminated. | ||
if (bool __is_identity = | ||
[&] [[__gnu__::__pure__]] { return std::ranges::find_first_of(__fmt, array{'{', '}'}) == __fmt.end(); }(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for one over the other?
9be4e43
to
0db3132
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
62712fc
to
7eb48db
Compare
d531697
to
7752a85
Compare
7752a85
to
e2cb0d1
Compare