Copilot commented on code in PR #49922:
URL: https://github.com/apache/arrow/pull/49922#discussion_r3346676299
##########
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
+ // (possibly using the multiply trick inside of xsimd).
+ // We use a variable left shift and fixed 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:
This replaces the project-specific `left_shift(...)` wrapper (which
previously provided SSE2/AVX2 fallbacks for smaller integer types, including
`uint8_t`) with `batch << kLShifts`. If xsimd still lacks a vector
variable-left-shift implementation for some `(Arch, Int)` combinations (notably
SSE2 with 8-bit lanes was explicitly handled before), this can become a
compile-time error or a silent behavior change. Recommendation (mandatory):
keep a minimal local fallback for the previously-special-cased widths/arches
(at least SSE2 `uint8_t`, and any other widths you were compensating for), or
gate the direct `<<` usage on an xsimd feature/version check that guarantees
correctness for those specific types.
##########
cpp/src/arrow/util/bpacking_simd_kernel_internal.h:
##########
@@ -1040,7 +962,7 @@ struct LargeKernel {
const auto high_swizzled = xsimd::swizzle(bytes, kHighSwizzles);
const auto high_words = xsimd::bitwise_cast<unpacked_type>(high_swizzled);
- const auto high_shifted = left_shift(high_words, kHighLShifts);
+ const auto high_shifted = high_words << kHighLShifts;
Review Comment:
Same concern as above: switching from the custom `left_shift(high_words,
kHighLShifts)` to `high_words << kHighLShifts` assumes xsimd 14.2 provides the
needed shift support for `unpacked_type` on all targeted architectures. If
`unpacked_type` can be `uint8_t`/`uint16_t` on SSE2/AVX2, this may fail to
compile or change semantics. Recommendation (mandatory): either restore the
earlier fallback path for the affected types/arches at this call site, or
constrain `unpacked_type`/architectures so that the operator shift is
guaranteed to be supported.
--
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]
