-
Notifications
You must be signed in to change notification settings - Fork 115
add simdutf constexpr more thoroughly #864
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
|
@pauldreik Please tell me what you think. |
pauldreik
left a comment
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 like this improvement! just some minor changes requested - constexpr on function declarations only needs C++11 constexpr (which we demand by requiring C++11). the new simdutf_constexpr macro only needs to be used on if statements.
src/scalar/swap_bytes.h
Outdated
|
|
||
| namespace utf32 { | ||
| template <endianness big_endian> | ||
| simdutf_constexpr uint32_t swap_if_needed(uint32_t c) { |
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 think this should be constexpr, not simdutf_constexpr
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. Done.
src/scalar/swap_bytes.h
Outdated
|
|
||
| namespace utf16 { | ||
| template <endianness big_endian> | ||
| simdutf_constexpr uint16_t swap_if_needed(uint16_t c) { |
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 think this should be constexpr, not simdutf_constexpr
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.
If I do so, I get...
/Users/dlemire/CVS/github/simdutf/src/scalar/swap_bytes.h:34:3: warning: use of this statement in a constexpr function is a C++14 extension [-Wc++14-extensions]
34 | if simdutf_constexpr (!match_system(big_endian)) {
| ^
/Users/dlemire/CVS/github/simdutf/src/scalar/swap_bytes.h:37:5: warning: multiple return statements in constexpr function is a C++14 extension [-Wc++14-extensions]
37 | return c;
|
|
||
| namespace simdutf { | ||
| namespace scalar { | ||
| namespace { |
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.
was this change intended and/or necessary?
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.
Without it, the build is broke, please see #869
src/scalar/utf16.h
Outdated
|
|
||
| // variable templates are a C++14 extension | ||
| template <endianness big_endian> char16_t replacement() { | ||
| template <endianness big_endian> simdutf_constexpr char16_t replacement() { |
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 think this should be an ordinary constexpr ( there are more places above with the same remark)
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.
@pauldreik I changed this one. What is allowed to be constexpr in C++11 is quite limited. So we can't just go and replace every simdutf_constexpr by a constexpr outside of the if constexpr expressions. However, I did a pass to try to extend the reach of the standard constexpr.
| int8x16x2_t pair = match_system(big_endian) | ||
| ? int8x16x2_t{{this->value, vmovq_n_s8(0)}} | ||
| : int8x16x2_t{{vmovq_n_s8(0), this->value}}; | ||
| simdutf_constexpr int8x16x2_t pair = |
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.
ordinary constexpr (since it is not an if)
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.
Fails with
/Users/dlemire/CVS/github/simdutf/src/simdutf/arm64/simd.h:313:27: fatal error: constexpr variable 'pair' must be initialized by a constant expression
313 | constexpr int8x16x2_t pair =
| ^
314 | match_system(big_endian) ? int8x16x2_t{{this->value, vmovq_n_s8(0)}}
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
315 | : int8x16x2_t{{vmovq_n_s8(0), this->value}};
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/westmere/sse_utf16fix.cpp
Outdated
| _mm_cmpeq_epi16(lb_masked, _mm_set1_epi16(swap_if_needed(0xd800U))); | ||
| block_is_low = | ||
| _mm_cmpeq_epi16(block_masked, _mm_set1_epi16(swap_if_needed(0xdc00U))); | ||
| lb_masked = _mm_and_si128( |
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.
just an opinion: I think the previous code was easier to read
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. I agree. Changed with a using so that it is not so busy. Did similar changes elsewhere.
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 keep forgetting that alias is only for types. Ok. Did it the long way around.
|
@pauldreik Should be better now. C++11 is quite limited, however. |
pauldreik
left a comment
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 think this is good to merge!
I also wonder if the compile time changes for the better or worse in C++17 builds with these changes?
Before I merge, can you elaborate on your concern ? Here is my belief: throwing in Do you have reasons to think otherwise? Of course we can measure, but it would be useful to have a mental model. In the past, I compared the compile time of major libraries as you switched from C++ 11 to C++17 to C++20. I never formalized my findings, but recollection is that for the same code base, exact same code, the compile time got worse as we went from C++11 to C++20. Thankfully, developers acquire much faster processors over time... So, in the real world, our code compiles faster. |
I agree it is good with a mental model! Unfortunately I don't have one. Instead I now measured before and after the change. The difference was within noise (gcc 14, debug, ninja, c++17).
Agree. |
|
Merging. This will be part of the next release. |
This is an extension of PR #859 by @anonrig
I picked up a comment by @pauldreik and ran with it.
This PR, just like #859 is not expected to affect the code's performance or functionality.
However, progressively extending the range of code that is
constexpris probably worthwhile.