Copilot commented on code in PR #49922:
URL: https://github.com/apache/arrow/pull/49922#discussion_r3339582031
##########
cpp/src/arrow/util/bpacking_simd_kernel_internal.h:
##########
@@ -262,27 +194,17 @@ ARROW_FORCE_INLINE auto right_shift_by_excess(
return xsimd::bitwise_cast<Int>(shifted0 | shifted1);
}
- // These conditions are the ones matched in `left_shift`, i.e. the ones
where variable
- // shift right will not be available but a left shift (fallback) exists.
+ // Architectures for which there is no variable right shift but a left shift
exists
+ // (eventually using the multiply trick).
+ // We use a variable left shift and fix right shift.
if constexpr (kIsSse2 && (IntSize != sizeof(uint64_t))) {
Review Comment:
The updated comment mentions 'eventually using the multiply trick' and
'variable left shift', but the multiply-trick implementation was removed along
with `left_shift(...)`, and this path now uses `batch << kLShifts`. Please
update the comment to reflect the current implementation to avoid misleading
future maintainers.
##########
cpp/src/arrow/util/bpacking_simd_kernel_internal.h:
##########
@@ -262,27 +194,17 @@ ARROW_FORCE_INLINE auto right_shift_by_excess(
return xsimd::bitwise_cast<Int>(shifted0 | shifted1);
}
- // These conditions are the ones matched in `left_shift`, i.e. the ones
where variable
- // shift right will not be available but a left shift (fallback) exists.
+ // Architectures for which there is no variable right shift but a left shift
exists
+ // (eventually using the multiply trick).
+ // We use a variable left shift and fix right shift.
if constexpr (kIsSse2 && (IntSize != sizeof(uint64_t))) {
constexpr Int kMaxRShift = max_value(std::array{kShifts...});
constexpr auto kLShifts =
xsimd::make_batch_constant<Int, kMaxRShift, Arch>() - shifts;
- return xsimd::bitwise_rshift<kMaxRShift>(left_shift(batch, kLShifts));
- }
-
- // TODO(xsimd) bug fixed in xsimd 14.1.0
- // https://github.com/xtensor-stack/xsimd/pull/1266
-#if XSIMD_VERSION_MAJOR < 14 || ((XSIMD_VERSION_MAJOR == 14) &&
XSIMD_VERSION_MINOR == 0)
- if constexpr (IsNeon<Arch>) {
- using SInt = std::make_signed_t<Int>;
- constexpr auto signed_shifts =
- xsimd::batch_constant<SInt, Arch, static_cast<SInt>(kShifts)...>();
- return xsimd::kernel::bitwise_rshift(batch, signed_shifts.as_batch(),
Arch{});
+ return xsimd::bitwise_rshift<kMaxRShift>(batch << kLShifts);
}
Review Comment:
With the removal of the `left_shift(...)` compatibility wrapper and the
xsimd version-guarded Neon fallback, this header now implicitly requires a
sufficiently new xsimd implementation of per-lane shifts. To avoid
hard-to-diagnose build failures for consumers who compile Arrow against an
older system xsimd, add an explicit compile-time version check (e.g., `#error`
/ `static_assert`) in a common header to enforce `XSIMD_VERSION >= 14.2.0` (or
the true minimum required by these kernels) with a clear message.
--
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]
