Skip to content
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.

Sign up for GitHub

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

Jump to bottom

MSVC optimizations for count_digits #1890

Merged
merged 1 commit into from
Sep 21, 2020
Merged

MSVC optimizations for count_digits #1890

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions include/fmt/format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ inline int clz(uint32_t x) {
// "r", but the only way that can happen is if "x" is 0,
// which the callers guarantee to not happen.
FMT_SUPPRESS_MSC_WARNING(6102)
return 31 - static_cast<int>(r);
return 31 ^ static_cast<int>(r);
}
# define FMT_BUILTIN_CLZ(n) detail::clz(n)

Expand All @@ -225,13 +225,13 @@ inline int clzll(uint64_t x) {
_BitScanReverse64(&r, x);
# else
// Scan the high 32 bits.
if (_BitScanReverse(&r, static_cast<uint32_t>(x >> 32))) return 63 - (r + 32);
if (_BitScanReverse(&r, static_cast<uint32_t>(x >> 32))) return 63 ^ (r + 32);
// Scan the low 32 bits.
_BitScanReverse(&r, static_cast<uint32_t>(x));
# endif
FMT_ASSERT(x != 0, "");
FMT_SUPPRESS_MSC_WARNING(6102) // Suppress a bogus static analysis warning.
return 63 - static_cast<int>(r);
return 63 ^ static_cast<int>(r);
}
# define FMT_BUILTIN_CLZLL(n) detail::clzll(n)

Expand Down Expand Up @@ -901,7 +901,7 @@ template <typename T = void> struct FMT_EXTERN_TEMPLATE_API basic_data {
// Maps bsr(n) to ceil(log10(pow(2, bsr(n) + 1) - 1)).
// This is a function instead of an array to workaround a bug in GCC10 (#1810).
FMT_INLINE uint16_t bsr2log10(int bsr) {
constexpr uint16_t data[] = {
static constexpr uint16_t data[] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm worried that this is going to cause GCC to trip up in the same way that #1810 identified. Note that MSVC isn't the only compiler to do this silliness with the data array, GCC does, too: https://gcc.godbolt.org/z/nbffbs. clang is unaffected (only a difference in the mangling). So if it does break GCC, can this at least be static everywhere except for when FMT_GCC_VERSION == 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried that this is going to cause GCC to trip up in the same way that #1810 identified.

Could you check it on godbolt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. I thought I was unable to reproduce the original issue (obviously isolated since godbolt's been updated with a "fixed" fmtlib), so I thought maybe GCC 10 had been fixed.
Got it to repro, and, no: https://godbolt.org/z/Ps4fod GCC 10 won't issue the string-op-overflow warnings with the data array being static. bsr2log10 also no longer requires always_inline to avoid the warning, as well. Which is just trivia, as there's no reason to remove it.

1, 1, 1, 2, 2, 2, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5,
6, 6, 6, 7, 7, 7, 7, 8, 8, 8, 9, 9, 9, 10, 10, 10,
10, 11, 11, 11, 12, 12, 12, 13, 13, 13, 13, 14, 14, 14, 15, 15,
Expand Down