Skip to content

Conversation

@lemire
Copy link
Member

@lemire lemire commented Nov 21, 2025

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 constexpr is probably worthwhile.

@lemire lemire requested a review from pauldreik November 22, 2025 03:27
@lemire
Copy link
Member Author

lemire commented Nov 22, 2025

@pauldreik Please tell me what you think.

Copy link
Collaborator

@pauldreik pauldreik left a 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.


namespace utf32 {
template <endianness big_endian>
simdutf_constexpr uint32_t swap_if_needed(uint32_t c) {
Copy link
Collaborator

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Done.


namespace utf16 {
template <endianness big_endian>
simdutf_constexpr uint16_t swap_if_needed(uint16_t c) {
Copy link
Collaborator

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

Copy link
Member Author

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 {
Copy link
Collaborator

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?

Copy link
Member Author

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


// variable templates are a C++14 extension
template <endianness big_endian> char16_t replacement() {
template <endianness big_endian> simdutf_constexpr char16_t replacement() {
Copy link
Collaborator

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)

Copy link
Member Author

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 =
Copy link
Collaborator

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)

Copy link
Member Author

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}};
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

_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(
Copy link
Collaborator

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

Copy link
Member Author

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.

Copy link
Member Author

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.

@lemire lemire requested a review from pauldreik December 4, 2025 05:05
@lemire
Copy link
Member Author

lemire commented Dec 4, 2025

@pauldreik Should be better now. C++11 is quite limited, however.

Copy link
Collaborator

@pauldreik pauldreik left a 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?

@lemire
Copy link
Member Author

lemire commented Dec 4, 2025

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 constexpr here and there should not affect compile time in a significant manner.

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.

@pauldreik
Copy link
Collaborator

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 ?

  • speaking for an improvement: if constexpr means the compiler can throw away the uninteresting branch early
  • speaking against an improvement: constexpr means stuff is evaluated in the "virtual abstract machine" in the compiler, which might be slow (it has only been around for ten years or so, while normal compilation has been done forever)

Here is my belief: throwing in constexpr here and there should not affect compile time in a significant manner.

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.

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).

Thankfully, developers acquire much faster processors over time... So, in the real world, our code compiles faster.

Agree.

@lemire
Copy link
Member Author

lemire commented Dec 4, 2025

Merging. This will be part of the next release.

@lemire lemire merged commit e6cb2b9 into master Dec 4, 2025
70 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants