pitrou commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3131111752
##########
cpp/src/arrow/util/bpacking_benchmark.cc:
##########
@@ -254,6 +254,60 @@ BENCHMARK_CAPTURE(BM_UnpackUint64, NeonUnaligned, false,
&bpacking::unpack_neon<
->ArgsProduct(kBitWidthsNumValues64);
#endif
+#if defined(ARROW_HAVE_RUNTIME_SVE128)
Review Comment:
I wonder if there's an easy way to reduce the duplication we're doing for
each runtime SIMD level?
For example if we could write something like:
```c++
BENCHMARK_SIMD_UNPACK(Bool, bool, SVE128, Sve128, sve128);
```
and it would expand to:
```c++
BENCHMARK_CAPTURE(BM_UnpackBool, Sve128Unaligned, false,
&bpacking::unpack_sve128<bool>,
!CpuInfo::GetInstance()->IsSupported(CpuInfo::SVE128),
"Sve128 not available")
->ArgsProduct(kBitWidthsNumValues<bool>);
```
##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -540,8 +563,9 @@ if(ARROW_CPU_FLAG STREQUAL "aarch64")
set(ARROW_HAVE_SVE_SIZELESS ON)
add_definitions(-DARROW_HAVE_SVE_SIZELESS)
endif()
+ else() # ARM v8 not SVE
Review Comment:
```suggestion
else() # ARM v8 without SVE
```
##########
cpp/src/arrow/util/bpacking_test.cc:
##########
@@ -191,168 +189,136 @@ class TestUnpack : public ::testing::TestWithParam<int>
{
EXPECT_EQ(unpacked, expected);
}
- template <typename Int>
void TestAll(UnpackFunc<Int> unpack) {
- const int num_values_base = GetParam();
-
- constexpr int kMaxBitWidth = std::is_same_v<Int, bool> ? 1 : 8 *
sizeof(Int);
-
- // Given how many edge cases there are in unpacking integers, it is best
to test all
- // sizes
- for (int bit_width = 0; bit_width <= kMaxBitWidth; ++bit_width) {
- SCOPED_TRACE(::testing::Message() << "Testing bit_width=" << bit_width);
-
- // We test all bit offset within a byte / misalignments to change how the
- // prolog.
- for (int bit_offset = 0; bit_offset < 8; ++bit_offset) {
- SCOPED_TRACE(::testing::Message() << "Testing bit_offset=" <<
bit_offset);
-
- const UnpackOptions opts{
- .batch_size = num_values_base,
- .bit_width = bit_width,
- .bit_offset = bit_offset,
- .max_read_bytes = -1, // No over-reading in testing (strict ASAN)
- };
-
- // Known values
- TestUnpackZeros(unpack, opts);
- TestUnpackOnes(unpack, opts);
- TestUnpackAlternating(unpack, opts);
-
- // Roundtrips
- TestRoundtripAlignment(unpack, opts);
-
- if (testing::Test::HasFailure()) return;
+ // There are actually many differences across the different sizes.
+ // It is best to test them all.
+ for (int num_values_base : {64, 128, 2048}) {
+ SCOPED_TRACE(::testing::Message() << "Testing num_values=" <<
num_values_base);
+
+ constexpr int kMaxBitWidth = std::is_same_v<Int, bool> ? 1 : 8 *
sizeof(Int);
+
+ // Given how many edge cases there are in unpacking integers, it is best
to test all
+ // sizes
Review Comment:
```suggestion
// Given how many edge cases there are in unpacking integers, it is
best to test all
// bit widths.
```
--
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]
