New bit manipulation functions and 128-bit value library #7338
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pdillinger
force-pushed
the
more_math
branch
from
6767c18
to
f010b5c
Compare
Summary: These new functions and 128-bit value bit operations are expected to be used in a forthcoming Bloom filter alternative. No functional changes to production code, just new code only called by unit tests, cosmetic changes to existing headers, and fix an existing function for a yet-unused template instantiation (BitsSetToOne on something signed and smaller than 32 bits). Test Plan: Unit tests included. Works with and without TEST_UINT128_COMPAT=1 to check compatibility with and without __uint128_t. Also added that parameter to the CircleCI build build-linux-shared_lib-alt_namespace-status_checked.
pdillinger
force-pushed
the
more_math
branch
from
f010b5c
to
7c27667
Compare
jay-zhuang
reviewed
Sep 2, 2020
util/math.h
Outdated
| } else if (sizeof(T) <= sizeof(unsigned long)) { | ||
| return __builtin_ctzl(static_cast<unsigned long>(v)); | ||
| } else { | ||
| return __builtin_ctzll(static_cast<unsigned long>(v)); |
There was a problem hiding this comment.
should it be static_cast<unsigned long long>?
util/math.h
Outdated
Comment on lines
33
to
39
| if (sizeof(T) <= sizeof(unsigned int)) { | ||
| int lz = __builtin_clz(static_cast<unsigned int>(v)); | ||
| return int{sizeof(unsigned int)} * 8 - 1 - lz; | ||
| } else { | ||
| int lz = __builtin_clzll(static_cast<unsigned long long>(v)); | ||
| return int{sizeof(unsigned long long)} * 8 - 1 - lz; | ||
| } |
There was a problem hiding this comment.
do we need to have __buildin_clzl() like other functions do? like:
else if (sizeof(T) <= sizeof(unsigned long)) {
return lz lz = __buildin_clzl(static_cast<unsigned long>(v));
} else {
util/math.h
Outdated
| } | ||
| #else | ||
| static_assert(sizeof(T) <= sizeof(unsigned long long), "type too big"); | ||
| if (sizeof(T) > sizeof(unsigned long)) { | ||
| return __builtin_popcountll(static_cast<unsigned long long>(v)); | ||
| } else if (sizeof(T) > sizeof(unsigned int)) { | ||
| return __builtin_popcountl(static_cast<unsigned long>(v)); | ||
| } else { | ||
| } else if (sizeof(T) == sizeof(unsigned int)) { | ||
| return __builtin_popcount(static_cast<unsigned int>(v)); |
There was a problem hiding this comment.
nit: maybe better to have a consistent ordering of checks for all functions. like from type size from small to big:
if (size < sizeof(unsigned int)) {
} else if (size == sizeof(unsigned int)) {
} else if (size <= sizeof(unsigned long)) {
} else {
}
util/math128.h
Outdated
| tmp += uint64_t{b & 0xffffFFFF} * uint64_t{a >> 32}; | ||
| // Avoid overflow: first add lower 32 of tmp2, and later upper 32 | ||
| uint64_t tmp2 = uint64_t{b >> 32} * uint64_t{a & 0xffffFFFF}; | ||
| tmp += static_cast<uint32_t>(tmp2); |
There was a problem hiding this comment.
nit: tmp += tmp2 & 0xffffFFFF; to be consistent with others?
jay-zhuang
approved these changes
Sep 2, 2020
There was a problem hiding this comment.
@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
jay-zhuang
reviewed
Sep 3, 2020
| uint64_t tmp = uint64_t{b & 0xffffFFFF} * uint64_t{a & 0xffffFFFF}; | ||
| uint64_t lower = tmp & 0xffffFFFF; | ||
| tmp >>= 32; | ||
| tmp += uint64_t{b & 0xffffFFFF} * uint64_t{a >> 32}; |
There was a problem hiding this comment.
It took me a while to figure out that it won't overflow 🤦
UINT_MAX * UINT_MAX + UINT_MAX + UINT_MAX = ULLONG_MAX
|
@pdillinger merged this pull request in c4d8838. |
codingrhythm
pushed a commit
to SafetyCulture/rocksdb
that referenced
this pull request
Mar 5, 2021
Summary: These new functions and 128-bit value bit operations are expected to be used in a forthcoming Bloom filter alternative. No functional changes to production code, just new code only called by unit tests, cosmetic changes to existing headers, and fix an existing function for a yet-unused template instantiation (BitsSetToOne on something signed and smaller than 32 bits). Pull Request resolved: facebook#7338 Test Plan: Unit tests included. Works with and without TEST_UINT128_COMPAT=1 to check compatibility with and without __uint128_t. Also added that parameter to the CircleCI build build-linux-shared_lib-alt_namespace-status_checked. Reviewed By: jay-zhuang Differential Revision: D23494945 Pulled By: pdillinger fbshipit-source-id: 5c0dc419100d9df5d4d9abb153b2855d5aea39e8
This was referenced Aug 30, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary: These new functions and 128-bit value bit operations are
expected to be used in a forthcoming Bloom filter alternative.
No functional changes to production code, just new code only called by
unit tests, cosmetic changes to existing headers, and fix an existing
function for a yet-unused template instantiation (BitsSetToOne on
something signed and smaller than 32 bits).
Test Plan: Unit tests included. Works with and without
TEST_UINT128_COMPAT=1 to check compatibility with and without
__uint128_t. Also added that parameter to the CircleCI build
build-linux-shared_lib-alt_namespace-status_checked.