This is an automated email from the ASF dual-hosted git repository.
pitrou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 5592d991939 GH-51492: [C++] Do not left shift a bpacking lane by its
full width (#51493)
5592d991939 is described below
commit 5592d99193950d615430c733550528aa8e66ccf6
Author: Dominique Belhachemi <[email protected]>
AuthorDate: Tue Sep 29 04:53:25 2026 -0400
GH-51492: [C++] Do not left shift a bpacking lane by its full width (#51493)
### Rationale for this change
When a packed value starts on a byte boundary its high part contributes
nothing, but LargeKernelPlan::Build still asks for a left shift, and on 8 bit
lanes that shift is the whole lane width. Some backends saturate and give zero,
others use the low bits of the shift amount and return the lane unchanged, so
its bits survive the mask.
So affected values decode incorrectly with no error reported, that can lead
to silent data corruption.
### What changes are included in this PR?
Point the high swizzle back at the low byte instead and set the shift to
zero. So the plan never asks for the shift at all.
This is a compile-time change to the kernel plan. The emitted kernel is
unchanged.
### Are these changes tested?
Yes
I also added a new assert that fails to compile on main.
### Are there any user-facing changes?
No
### Was AI used for this PR?
**PR code and description written by:**
- [x] Human
- [x] AI
**Reviewed before submission by:**
- [x] Human
- [x] AI
- [ ] Not reviewed
* GitHub Issue: #51492
Authored-by: Dominique Belhachemi <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
---
cpp/src/arrow/util/bpacking_simd_kernel_internal.h | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/cpp/src/arrow/util/bpacking_simd_kernel_internal.h
b/cpp/src/arrow/util/bpacking_simd_kernel_internal.h
index 83a969cca64..0c4b56eaa8b 100644
--- a/cpp/src/arrow/util/bpacking_simd_kernel_internal.h
+++ b/cpp/src/arrow/util/bpacking_simd_kernel_internal.h
@@ -809,6 +809,8 @@ constexpr auto LargeKernelPlan<KerTraits>::Build() ->
LargeKernelPlan<KerTraits>
// const int packed_end_bit = packed_start_bit +
kShape.packed_bit_size();
// const int packed_end_byte = (packed_end_bit - 1) / 8 + 1;
+ const bool byte_aligned = packed_start_bit % 8 == 0;
+
// Looping over maximum number of bytes that can fit a value
// We fill more than necessary in the high swizzle because in the
absence of
// variable right shifts, we will erase some bits from the low sizzled
values.
@@ -825,7 +827,12 @@ constexpr auto LargeKernelPlan<KerTraits>::Build() ->
LargeKernelPlan<KerTraits>
// explicit here.
// We need to stay pessimistic with ``kOverBytes`` to avoid having a
mix of right
// and left shifts in the high swizzle.
+ // When the value is byte aligned we point the high swizzle back at
the low byte
+ // instead, and pair it with a zero lshift below.
auto high_swizzle_val = low_swizzle_val + kOverBytes;
+ if (byte_aligned) {
+ high_swizzle_val = low_swizzle_val;
+ }
if (high_swizzle_val >= kShape.simd_byte_size()) {
high_swizzle_val = kUndefined;
}
@@ -837,9 +844,19 @@ constexpr auto LargeKernelPlan<KerTraits>::Build() ->
LargeKernelPlan<KerTraits>
}
// low and high swizzles need to be rshifted but the oversized bytes
create a
- // larger lshift for high values.
+ // larger lshift for high values. A byte aligned value has no high part,
so both
+ // swizzles hold the same byte and the lshift is zero. Left-shifting the
high byte
+ // out by the whole lane width would also remove it, but that shift
gives zero on
+ // some instruction sets and leaves the lane untouched on others, where
its bits
+ // then survive the mask.
plan.low_rshifts.at(r).at(u) = packed_start_bit % 8;
- plan.high_lshifts.at(r).at(u) = 8 * kOverBytes - (packed_start_bit % 8);
+ if (byte_aligned) {
+ plan.high_lshifts.at(r).at(u) = 0;
+ } else {
+ plan.high_lshifts.at(r).at(u) = 8 * kOverBytes - (packed_start_bit %
8);
+ }
+ assert(static_cast<int>(plan.high_lshifts.at(r).at(u)) <
+ kShape.unpacked_bit_size());
packed_start_bit += kShape.packed_bit_size();
}
