Copilot commented on code in PR #49922:
URL: https://github.com/apache/arrow/pull/49922#discussion_r3339914683
##########
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 change removes the previous xsimd-version-conditional workaround (and
the custom `left_shift`) and now assumes `batch << batch_constant` is
correct/available for all supported xsimd versions/arches. However, the repo
still appears to version xsimd in multiple ways (bundled vs system/consumer
builds). To prevent silent miscompiles for consumers building with older xsimd
(e.g., 14.0.x), consider enforcing a minimum xsimd version in headers/build
config (e.g., a compile-time `#error`/`static_assert` on `XSIMD_VERSION_*`) or
keep the old workaround behind a version guard.
##########
ci/conda_env_cpp.txt:
##########
@@ -47,6 +47,6 @@ rapidjson
re2
snappy
thrift-cpp>=0.11.0
-xsimd>=14.0
+xsimd>=14.2
Review Comment:
Using `xsimd>=14.2` allows automatic upgrades to a future major release
(e.g., 15.x) which could introduce breaking changes and cause unexpected CI or
packaging failures. Consider constraining the upper bound (e.g., `>=14.2,<15`)
to keep CI reproducible and aligned with the explicitly pinned third-party
version.
--
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]
