Vishwanatha-HD commented on code in PR #48180:
URL: https://github.com/apache/arrow/pull/48180#discussion_r2545344185
##########
cpp/src/arrow/compute/util.cc:
##########
@@ -30,34 +30,40 @@ namespace util {
namespace bit_util {
inline uint64_t SafeLoadUpTo8Bytes(const uint8_t* bytes, int num_bytes) {
- // This will not be correct on big-endian architectures.
-#if !ARROW_LITTLE_ENDIAN
- ARROW_DCHECK(false);
-#endif
ARROW_DCHECK(num_bytes >= 0 && num_bytes <= 8);
if (num_bytes == 8) {
return util::SafeLoad(reinterpret_cast<const uint64_t*>(bytes));
Review Comment:
Thanks for pointing out this.. Now with the way we are handling the
tail_bytes and loading the word data, we dont actually need to change
"SafeLoadUpTo8Bytes()" function.. With the conditional compilation, this
function will never be called on Big-endian architecture.
I have reverted this change.. Tested completely on s390x to see if all the
test work. I have pushed a new commit. Please give your review comments. Thanks.
##########
cpp/src/arrow/compute/util.cc:
##########
@@ -118,7 +124,22 @@ void bits_to_indexes_internal(int64_t hardware_flags,
const int num_bits,
// Optionally process the last partial word with masking out bits outside
range
if (tail) {
const uint8_t* bits_tail = bits + (num_bits - tail) / 8;
+#if ARROW_LITTLE_ENDIAN
uint64_t word = SafeLoadUpTo8Bytes(bits_tail, (tail + 7) / 8);
+#else
+ int tail_bytes = (tail + 7) / 8;
+ uint64_t word;
+ if (tail_bytes == 8) {
+ word = util::SafeLoad(reinterpret_cast<const uint64_t*>(bits_tail));
+ } else {
+ // For bit manipulation, always load into least significant bits
+ // to ensure compatibility with CountTrailingZeros on Big-endian systems
+ word = 0;
+ for (int i = 0; i < tail_bytes; ++i) {
+ word |= static_cast<uint64_t>(bits_tail[i]) << (8 * i);
+ }
+ }
+#endif
Review Comment:
Now that I have removed the big-endian support to SafeLoadUpTo8Bytes()
function, these changes are required as these handle the way we handle the
tail_bytes on big-endian systems. If the tail_bytes are equal to 8, then we
call directly the SafeLoad to load the data onto "word" variable. And for rest
other cases, we need to take care of loading least significant bits to ensure
compatibility with "CountTrailingZeros". This is the reason why we wont be able
to make a direct call "SafeLoadUpTo8Bytes()" for every tail_bytes.
##########
cpp/src/arrow/compute/util.cc:
##########
@@ -118,7 +124,22 @@ void bits_to_indexes_internal(int64_t hardware_flags,
const int num_bits,
// Optionally process the last partial word with masking out bits outside
range
if (tail) {
const uint8_t* bits_tail = bits + (num_bits - tail) / 8;
+#if ARROW_LITTLE_ENDIAN
uint64_t word = SafeLoadUpTo8Bytes(bits_tail, (tail + 7) / 8);
+#else
+ int tail_bytes = (tail + 7) / 8;
+ uint64_t word;
+ if (tail_bytes == 8) {
+ word = util::SafeLoad(reinterpret_cast<const uint64_t*>(bits_tail));
+ } else {
+ // For bit manipulation, always load into least significant bits
+ // to ensure compatibility with CountTrailingZeros on Big-endian systems
+ word = 0;
+ for (int i = 0; i < tail_bytes; ++i) {
+ word |= static_cast<uint64_t>(bits_tail[i]) << (8 * i);
+ }
+ }
+#endif
Review Comment:
I have fixed the lint errors and pushed my changes. Thanks..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
