-
Notifications
You must be signed in to change notification settings - Fork 76
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
Implement a c++20 constexpr type demangler #2472
base: develop
Are you sure you want to change the base?
Conversation
925d205
to
51449bb
Compare
51449bb
to
2ae434c
Compare
#if BOOST_COMP_CLANG | ||
# pragma clang diagnostic pop | ||
#endif | ||
inline constexpr std::string demangled = std::string(demangle<T>()); |
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.
This should be rewritten using an std::string_view
rather than an std::string
- but it requires changes in many other places, I'll take care in a second moment.
6c62bb8
to
5b621d2
Compare
Looks like event with |
I really hate clang pointless warnings about perfectly valid C++ being treated as errors:
|
355b6a6
to
ffd5c5d
Compare
Same for
|
ebde117
to
2824b49
Compare
# ifdef __cpp_lib_format | ||
std::format("AccCpuOmp2Blocks<{},{}>", TDim::value, core::demangled<TIdx>); | ||
# else | ||
"AccCpuOmp2Blocks<"s + std::to_string(TDim::value) + ","s + std::string(core::demangled<TIdx>) |
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 std::stringstream
maybe a better solution?
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.
I can give it a try.
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.
I don't know, if std::stringstream
has a performance disadvantage compare to std::format
. I need to check it. But if not, I would suggest it.
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.
I was a little bit searching. std::format
should be faster by factors and concatenating std::string
should be faster than std::stringstream
. The benefit of std::stringstream
is the robustness for different data types.
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.
This should work:
static std::string const accName =
# ifdef __cpp_lib_format
std::format("AccCpuOmp2Blocks<{},{}>", TDim::value, core::demangled<TIdx>);
# else
(std::stringstream() << "AccCpuOmp2Blocks<" << TDim::value << "," << core::demangled<TIdx> << ">").str();
# endif
Do you prefer to keep the optional use of std::format
, or go with std::stringstream
in any case ?
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.
I also thought about this idea, but than the PR does not make sense anymore, because the PR should remove a dependency and not replace it.
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.
I don't mind having boost as a dependency, so for me improvement is the constexpr
functionality.
However, unlike boost I think {fmt} is header only, and we could include it directly in alpaka as a third party library like catch and mdspan.
Anyway, it's just an idea, and in case we can follow up in a later PR.
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.
I don't mind having boost as a dependency, so for me improvement is the
constexpr
functionality.
Ah okay. I thought this PR is pulling out a part of this PR #2409. But this was a misunderstanding from my side (to much different stuff in the morning ;-) ).
However, unlike boost I think {fmt} is header only, and we could include it directly in alpaka as a third party library like catch and mdspan.
Anyway, it's just an idea, and in case we can follow up in a later PR.
Fair enough. We need only to support one specific version, which we can ship via sub directory or via CMake. We can discuss this tomorrow in the VC.
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.
During the VC we discussed, if you should use the fmt library. @psychocoderHPC is against it, because the advantage of using it in a debug function is not big enough for the extra work caused by maintaining a dependency.
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.
Unfortunately gcc 10 does not like the ostringstream
-based syntax (it works with gcc 11 and later, and all clang+ versions I tried).
I'll revert to
... = "AccCpuOmp2Blocks<"s + std::to_string(TDim::value) + ","s + std::string(core::demangled<TIdx>);
2824b49
to
b8b0985
Compare
@@ -162,7 +166,21 @@ namespace alpaka | |||
{ | |||
ALPAKA_FN_HOST static auto getAccName() -> std::string |
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.
The PR headline mentioned a constexpr
type demangler. Is it possible to make the function constexpr
or consteval
? In this case the performance of std::format
and std::stringstream
does not matter.
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.
I didn't find a way.
The performance doesn't matter much anyway, because the name is computed only once, the first time it's used.
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 the moment I play a little bit around with it but it is not easy that compiler really optimize.
But if the performance does not matter, we simply keep the simple implementation and optimize it, if become an bottleneck.
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.
Okay, I found a major problem, if we want to make it constexpr
. C++23 is required for static variables in constexpr
functions.
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.
Yes, it's the same issue I had to work around here:
// Store the demangled type name as a null-terminated array of bytes.
// Note: this could be a function-static constexpr variable in c++23.
template<typename T>
inline constexpr auto storage = demangleAsArray<T>();
Maybe we can use a consteval
function and store it as a static constexpr
data member.
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.
During the VC today we talked about the subject. It is not performance critical. Therefore we can keep it as it is.
b8b0985
to
fe19cda
Compare
The fallback source location implementation for demange equal to #2405? |
# ifdef __cpp_lib_format | ||
std::format("AccGpu{}Rt<{},{}>", TApi::name, TDim::value, core::demangled<TIdx>); | ||
# else | ||
(std::ostringstream() |
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.
To workaround the nvcc error here
error: class "std::basic_ostream<char, std::char_traits<char>>" has no member "str"
.str();
^
I would try if std::string(<ostringstream>)
works
// Store the demangled type name as a null-terminated array of bytes. | ||
// Note: this could be a function-static constexpr variable in c++23. | ||
template<typename T> | ||
inline constexpr auto storage = demangleAsArray<T>(); |
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.
/builds/hzdr/crp/alpaka/include/alpaka/core/DemangleTypeNames.hpp(78): error: call to consteval function "<unnamed>::demangleAsArray<T>() [with T=uint32_t]" did not produce a valid constant expression
/builds/hzdr/crp/alpaka/include/alpaka/core/DemangleTypeNames.hpp(72): note #2751-D: access to uninitialized object
Maybe using inline ALPAKA_FN_HOST constexpr auto storage = demangleAsArray<T>();
solve the issue
29a8abc
to
17523d9
Compare
17523d9
to
4a223b3
Compare
Replace the run-time type demangler based on Boost with a constexpr version based on embedding the type name in the compiler generated function name.
Based on https://www.reddit.com/r/cpp/comments/lfi6jt/finally_a_possibly_portable_way_to_convert_types/ and https://rodusek.com/posts/2021/03/09/getting-an-unmangled-type-name-at-compile-time/ .