-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Workaround intel bug #3652
Workaround intel bug #3652
Conversation
Potential workaround / restructure for the intel bug that is the cause of fmtlib#3645. Make the variable in the external struct instead an embedded static constexpr variable in the only function that uses the variable.
Pull request not correct for certain c++ std levels. Will let someone with more knowledge propose a solution. |
include/fmt/format.h
Outdated
// For checking rounding thresholds. | ||
// The kth entry is chosen to be the smallest integer such that the | ||
// upper 32-bits of 10^(k+1) times it is strictly bigger than 5 * 10^k. | ||
static constexpr uint32_t fractional_part_rounding_thresholds[8] = { |
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.
You need to drop static here:
/Users/runner/work/fmt/fmt/include/fmt/format.h:3268:29: error: definition of a static variable in a constexpr function is a C++2b extension [-Werror,-Wc++2b-extensions]
static constexpr uint32_t fractional_part_rounding_thresholds[8] = {
^
Also please move to a separate function to avoid parametrization on Float
, something like
FMT_CONSTEXPR uint32_t fractional_part_rounding_threshold(int index) {
constexpr uint32_t thresholds[] = {...};
return thresholds[index];
}
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.
@vitaut
Will a non-static constexpr array be optimized by the compiler?
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.
¯_(ツ)_/¯
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.
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.
Done.
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.
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 think we should revert this PR but ideas/PRs to optimize it would be welcome.
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.
@vitaut
Solution for GCC, Clang: https://godbolt.org/z/ezj9GhnYW
But not for MSVC (current code: https://godbolt.org/z/5Pxn3zMfT)
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 have another idea: 06b2038.
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.
@vitaut Solution for GCC, Clang: https://godbolt.org/z/ezj9GhnYW But not for MSVC (current code: https://godbolt.org/z/5Pxn3zMfT)
That looks good. I had something similar but didn't think of the unreachable
to quiet the warnings and keep the optimization.
Moved variable out of function to avoid specialization on Float. Made it a separate function that is called from format_float.
Thanks |
* Workaround intel bug Potential workaround / restructure for the intel bug that is the cause of fmtlib#3645. Make the variable in the external struct instead an embedded static constexpr variable in the only function that uses the variable. * Finish the proposed change -- remove struct accessor * Refactor proposed intel fix. Moved variable out of function to avoid specialization on Float. Made it a separate function that is called from format_float. * Fix incorrect function name. * Add missing inline.
Potential workaround / restructure for the intel bug that is the cause of #3645.
Make the variable in the external struct instead an embedded static constexpr variable in the only function that uses the variable.