-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
constexpr all the generate_canonical parameters #2498
Conversation
...up to 64 bits of entropy. Leave the original implementation for more bits and naughty generators (I'm looking at you, tr1) whose min and max functions aren't static. Fixes microsoft#1964. Alternative to microsoft#2452.
tests/std/tests/GH_001964_constexpr_generate_canonical/test.cpp
Outdated
Show resolved
Hide resolved
@AlexGuteniev It looks like #2343 made |
I think this I did on my own ant it is intentional to make Though there are some ways to restore it:
#ifndef __CUDACC__
_NODISCARD constexpr bool _Is_constant_evaluated() noexcept { // Internal function for any standard mode
return __builtin_is_constant_evaluated();
}
#endif // __CUDACC__ I generally think the last option is the way to go, as going further with optimization we will eventually need this anyway. |
@MattStephanson @AlexGuteniev As the only call in product code is to create a That said, since this code is at compile-time, the only reason to have the power-of-two special case is for throughput. Does this make a measurable difference, or prevent errors like the constexpr step limit? If there's no observable throughput difference and no correctness impact, then the power-of-two special case is just more code to reason about, and I would suggest eliminating it. |
I haven't benchmarked it, but it does seem cleaner to just eliminate that branch. That does require special-casing the |
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.
Verr nice!
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.
Thanks! The issues I found were small/nitpicky so I'll go ahead and validate/push changes.
tests/std/tests/GH_001964_constexpr_generate_canonical/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/GH_001964_constexpr_generate_canonical/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/GH_001964_constexpr_generate_canonical/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/GH_001964_constexpr_generate_canonical/test.cpp
Outdated
Show resolved
Hide resolved
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Thanks for picking up and generalizing my lazy special-case fix! |
...up to 64 bits of entropy. Leave the original implementation for
more bits and naughty generators (I'm looking at you, tr1) whose
min and max functions aren't static.
Fixes #1964. Alternative to #2452.
A slightly modified version of @CaseyCarter's release (
/O2 /fp:strict
for me) codegen test:asm diff
Note that in the case when the range is 10,001, not a power-of-two, the number of iterations is a hard-coded 4.