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

GH-41687: [C++] bit_util: Trying to remove pre-compute table #41690

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+28 −11
Open

GH-41687: [C++] bit_util: Trying to remove pre-compute table #41690

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e2f513e
bit-util-remove-pre-compute
mapleFU May 16, 2024
2eb38e4
apply suggestions
mapleFU May 17, 2024
064a328
more compiler assume
mapleFU May 20, 2024
270e1a2
Remove index <= 7
mapleFU May 21, 2024
4a9bae8
Merge branch 'main' into arrow-bit-util-remove-precompute-table
mapleFU May 24, 2024
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: 6 additions & 2 deletions cpp/src/arrow/util/bit_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,15 +282,19 @@ static inline int Log2(uint64_t x) {

// Bitmask selecting the k-th bit in a byte
// static constexpr uint8_t kBitmask[] = {1, 2, 4, 8, 16, 32, 64, 128};
static constexpr uint8_t GetBitMask(uint8_t index) {
template <typename T>
static constexpr uint8_t GetBitMask(T index) {
// DCHECK(index >= 0 && index <= 7);
ARROW_COMPILER_ASSUME(index >= 0 && index <= 7);
return static_cast<uint8_t>(1) << index;
Copy link
Contributor

Choose a reason for hiding this comment

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

The compiler will generate code that performs the << on a 32-bit integer. A more honest implementation (in the sense that it gives more freedom to the compiler [1]):

return static_cast<uint8_t>(0x1 << index)
// and
return static_cast<uint8_t>(~(0x1 << i))

And since indices in arrow are rarely uint8_t, I would keep the index type unconstrained like this:

template <typename T>
static constexpr uint8_t GetBitmask2(T index) {
    return static_cast<uint8_t>(0x1 << index);
}
template <typename T>
static constexpr uint8_t GetFlippedBitmask2(T index) {
  return static_cast<uint8_t>(~(0x1 << index));
}

[1] might matter more for rustc than clang because C/C++ compilers already have a lot of freedom even when your code contains many type constraints

}

// the bitwise complement version of kBitmask
// static constexpr uint8_t kFlippedBitmask[] = {254, 253, 251, 247, 239, 223, 191, 127};
static constexpr uint8_t GetFlippedBitMask(uint8_t index) {
template <typename T>
static constexpr uint8_t GetFlippedBitMask(T index) {
// DCHECK(index >= 0 && index <= 7);
ARROW_COMPILER_ASSUME(index >= 0 && index <= 7);
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting the assumption here doesn't have the same effect. Because signed_index % 8 happens before the call to GetBitMask and GetFlippedBitMask.

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'm not fully understand this, signed_index % 8 happens before the call to GetBitmask doesn't means the code of GetBitMask cannot benifit from this? Or you means the system might analyze % 8 and do the assume itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or from the godbolt link, do you mean change ClearBit and SetBit to ub if i < 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, tried in godbolt, I'll add this

Copy link
Contributor

Choose a reason for hiding this comment

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

ARROW_COMPILER_ASSUME(index >= 0 && index <= 7) is superfluous because shifts are already UB in C when the size is bigger than the bit-width of the integer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha, I use ARROW_COMPILER_ASSUME(index >= 0 && index <= 7) like DCHECK here, removed it

return ~(static_cast<uint8_t>(1) << index);
}

Expand Down
Loading