This is an automated email from the ASF dual-hosted git repository. raulcd pushed a commit to branch maint-25.0.x in repository https://gitbox.apache.org/repos/asf/arrow.git
commit 149158986337257eed43392ee04a2680504de77c Author: Antoine Prouvost <[email protected]> AuthorDate: Thu Jul 30 15:05:45 2026 +0200 GH-50503: [Parquet] Remove SVE128 unpack (#50611) ### Rationale for this change The SVE128 code path has conflict with the SVE256 that we do not yet manage properly. - There was first the ODR violation in GH-49921 - Now it seems that there may also be an issue with LTO Anyhow, after we fixed the inlining issue in Neon, the SVE128 had no clear advantages over Neon as expected, os this was due to be removed anyways. ### What changes are included in this PR? Remove SVE128 unpack ### Are these changes tested? In CI. ### Are there any user-facing changes? No * GitHub Issue: #50503 Lead-authored-by: AntoinePrv <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]> --- cpp/src/arrow/CMakeLists.txt | 28 +++++++++++++--- cpp/src/arrow/util/bpacking.cc | 7 ++-- cpp/src/arrow/util/bpacking_benchmark.cc | 5 --- cpp/src/arrow/util/bpacking_simd_128_alt.cc | 51 ----------------------------- cpp/src/arrow/util/bpacking_simd_internal.h | 28 ---------------- cpp/src/arrow/util/bpacking_test.cc | 9 ----- 6 files changed, 28 insertions(+), 100 deletions(-) diff --git a/cpp/src/arrow/CMakeLists.txt b/cpp/src/arrow/CMakeLists.txt index 8750598f6c..c8c1de1c85 100644 --- a/cpp/src/arrow/CMakeLists.txt +++ b/cpp/src/arrow/CMakeLists.txt @@ -346,21 +346,42 @@ endmacro() macro(append_runtime_sve128_src SRCS SRC) if(ARROW_HAVE_RUNTIME_SVE128) list(APPEND ${SRCS} ${SRC}) - set_source_files_properties(${SRC} PROPERTIES COMPILE_OPTIONS "${ARROW_SVE128_FLAGS}") + set(_flags ${ARROW_SVE128_FLAGS}) + if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + # Disable LTO to work around GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=121412 + list(APPEND ${_flags} "-fno-lto") + endif() + set_property(SOURCE ${SRC} + APPEND + PROPERTY COMPILE_OPTIONS ${_flags}) endif() endmacro() macro(append_runtime_sve256_src SRCS SRC) if(ARROW_HAVE_RUNTIME_SVE256) list(APPEND ${SRCS} ${SRC}) - set_source_files_properties(${SRC} PROPERTIES COMPILE_OPTIONS "${ARROW_SVE256_FLAGS}") + set(_flags ${ARROW_SVE256_FLAGS}) + if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + # Disable LTO to work around GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=121412 + list(APPEND ${_flags} "-fno-lto") + endif() + set_property(SOURCE ${SRC} + APPEND + PROPERTY COMPILE_OPTIONS ${_flags}) endif() endmacro() macro(append_runtime_sve512_src SRCS SRC) if(ARROW_HAVE_RUNTIME_SVE512) list(APPEND ${SRCS} ${SRC}) - set_source_files_properties(${SRC} PROPERTIES COMPILE_OPTIONS "${ARROW_SVE512_FLAGS}") + set(_flags ${ARROW_SVE512_FLAGS}) + if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + # Disable LTO to work around GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=121412 + list(APPEND ${_flags} "-fno-lto") + endif() + set_property(SOURCE ${SRC} + APPEND + PROPERTY COMPILE_OPTIONS ${_flags}) endif() endmacro() @@ -588,7 +609,6 @@ append_runtime_avx2_src(ARROW_UTIL_SRCS util/byte_stream_split_internal_avx2.cc) append_runtime_avx2_src(ARROW_UTIL_SRCS util/bpacking_simd_256.cc) append_runtime_avx512_src(ARROW_UTIL_SRCS util/bpacking_simd_avx512.cc) -append_runtime_sve128_src(ARROW_UTIL_SRCS util/bpacking_simd_128_alt.cc) append_runtime_sve256_src(ARROW_UTIL_SRCS util/bpacking_simd_256.cc) if(ARROW_WITH_BROTLI) diff --git a/cpp/src/arrow/util/bpacking.cc b/cpp/src/arrow/util/bpacking.cc index 1bf81df4f2..b299273456 100644 --- a/cpp/src/arrow/util/bpacking.cc +++ b/cpp/src/arrow/util/bpacking.cc @@ -32,9 +32,10 @@ struct UnpackDynamicFunction { static constexpr auto targets() { return std::array{ - ARROW_DISPATCH_TARGET_NONE(&bpacking::unpack_scalar<Uint>) // - ARROW_DISPATCH_TARGET_NEON(&bpacking::unpack_neon<Uint>) // - ARROW_DISPATCH_TARGET_SVE128(&bpacking::unpack_sve128<Uint>) // + ARROW_DISPATCH_TARGET_NONE(&bpacking::unpack_scalar<Uint>) // + ARROW_DISPATCH_TARGET_NEON(&bpacking::unpack_neon<Uint>) // + // GH-50503: No SVE128 dispatch as it increases code size without + // increasing performance vs. Neon, and can produce ODR violations. ARROW_DISPATCH_TARGET_SVE256(&bpacking::unpack_sve256<Uint>) // ARROW_DISPATCH_TARGET_SSE4_2(&bpacking::unpack_sse4_2<Uint>) // ARROW_DISPATCH_TARGET_AVX2(&bpacking::unpack_avx2<Uint>) // diff --git a/cpp/src/arrow/util/bpacking_benchmark.cc b/cpp/src/arrow/util/bpacking_benchmark.cc index 93d7cdf165..025b493c59 100644 --- a/cpp/src/arrow/util/bpacking_benchmark.cc +++ b/cpp/src/arrow/util/bpacking_benchmark.cc @@ -206,11 +206,6 @@ BENCHMARK_UNPACK_ALL_TYPES_RUNTIME(Avx512Unaligned, false, bpacking::unpack_avx5 BENCHMARK_UNPACK_ALL_TYPES(NeonUnaligned, false, bpacking::unpack_neon); #endif -#if defined(ARROW_HAVE_RUNTIME_SVE128) -BENCHMARK_UNPACK_ALL_TYPES_RUNTIME(Sve128Unaligned, false, bpacking::unpack_sve128, - SVE128, "Sve128 not available"); -#endif - #if defined(ARROW_HAVE_RUNTIME_SVE256) BENCHMARK_UNPACK_ALL_TYPES_RUNTIME(Sve256Unaligned, false, bpacking::unpack_sve256, SVE256, "Sve256 not available"); diff --git a/cpp/src/arrow/util/bpacking_simd_128_alt.cc b/cpp/src/arrow/util/bpacking_simd_128_alt.cc deleted file mode 100644 index bd4799d3cd..0000000000 --- a/cpp/src/arrow/util/bpacking_simd_128_alt.cc +++ /dev/null @@ -1,51 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, -// software distributed under the License is distributed on an -// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -// KIND, either express or implied. See the License for the -// specific language governing permissions and limitations -// under the License. - -#if defined(ARROW_HAVE_RUNTIME_SVE128) -# define UNPACK_PLATFORM unpack_sve128 -# define KERNEL_PLATFORM KernelSve128 -#endif - -#if !defined(UNPACK_PLATFORM) -# error "This file must be compiled with a known SIMD micro architecture" -#endif - -#include <xsimd/xsimd.hpp> - -#include "arrow/util/bpacking_dispatch_internal.h" -#include "arrow/util/bpacking_simd_internal.h" -#include "arrow/util/bpacking_simd_kernel_internal.h" - -namespace arrow::internal::bpacking { - -template <typename UnpackedUint, int kPackedBitSize> -using KERNEL_PLATFORM = Kernel<UnpackedUint, kPackedBitSize, xsimd::detail::sve<128>>; - -template <typename Uint> -void UNPACK_PLATFORM(const uint8_t* in, Uint* out, const UnpackOptions& opts) { - return unpack_jump<KERNEL_PLATFORM>(in, out, opts); -} - -template void UNPACK_PLATFORM<bool>(const uint8_t*, bool*, const UnpackOptions&); -template void UNPACK_PLATFORM<uint8_t>(const uint8_t*, uint8_t*, const UnpackOptions&); -template void UNPACK_PLATFORM<uint16_t>(const uint8_t*, uint16_t*, const UnpackOptions&); -template void UNPACK_PLATFORM<uint32_t>(const uint8_t*, uint32_t*, const UnpackOptions&); -template void UNPACK_PLATFORM<uint64_t>(const uint8_t*, uint64_t*, const UnpackOptions&); - -} // namespace arrow::internal::bpacking - -#undef UNPACK_PLATFORM diff --git a/cpp/src/arrow/util/bpacking_simd_internal.h b/cpp/src/arrow/util/bpacking_simd_internal.h index d5a81baaec..78aaa4a8f9 100644 --- a/cpp/src/arrow/util/bpacking_simd_internal.h +++ b/cpp/src/arrow/util/bpacking_simd_internal.h @@ -53,34 +53,6 @@ extern template ARROW_TEMPLATE_EXPORT void UNPACK_ARCH128<uint64_t>( #endif // UNPACK_ARCH128 #undef UNPACK_ARCH128 -#if defined(ARROW_HAVE_RUNTIME_SVE128) -# define UNPACK_ARCH128_ALT unpack_sve128 -#endif - -#if defined(UNPACK_ARCH128_ALT) - -template <typename Uint> -ARROW_EXPORT void UNPACK_ARCH128_ALT(const uint8_t* in, Uint* out, - const UnpackOptions& opts); - -extern template ARROW_TEMPLATE_EXPORT void UNPACK_ARCH128_ALT<bool>( // - const uint8_t* in, bool* out, const UnpackOptions& opts); - -extern template ARROW_TEMPLATE_EXPORT void UNPACK_ARCH128_ALT<uint8_t>( - const uint8_t* in, uint8_t* out, const UnpackOptions& opts); - -extern template ARROW_TEMPLATE_EXPORT void UNPACK_ARCH128_ALT<uint16_t>( - const uint8_t* in, uint16_t* out, const UnpackOptions& opts); - -extern template ARROW_TEMPLATE_EXPORT void UNPACK_ARCH128_ALT<uint32_t>( - const uint8_t* in, uint32_t* out, const UnpackOptions& opts); - -extern template ARROW_TEMPLATE_EXPORT void UNPACK_ARCH128_ALT<uint64_t>( - const uint8_t* in, uint64_t* out, const UnpackOptions& opts); - -#endif // UNPACK_ARCH128_ALT -#undef UNPACK_ARCH128_ALT - #if defined(ARROW_HAVE_SVE256) || defined(ARROW_HAVE_RUNTIME_SVE256) # define UNPACK_ARCH256 unpack_sve256 #elif defined(UNPACK_ARCH256) || defined(ARROW_HAVE_RUNTIME_AVX2) diff --git a/cpp/src/arrow/util/bpacking_test.cc b/cpp/src/arrow/util/bpacking_test.cc index d4d588228e..0503a15110 100644 --- a/cpp/src/arrow/util/bpacking_test.cc +++ b/cpp/src/arrow/util/bpacking_test.cc @@ -301,15 +301,6 @@ TYPED_TEST(TestUnpack, UnpackAvx512) { TYPED_TEST(TestUnpack, UnpackNeon) { this->TestAll(&bpacking::unpack_neon<TypeParam>); } #endif -#if defined(ARROW_HAVE_RUNTIME_SVE128) -TYPED_TEST(TestUnpack, UnpackSve128) { - if (!CpuInfo::GetInstance()->IsSupported(CpuInfo::SVE128)) { - GTEST_SKIP() << "Test requires SVE128"; - } - this->TestAll(&bpacking::unpack_sve128<TypeParam>); -} -#endif - #if defined(ARROW_HAVE_RUNTIME_SVE256) TYPED_TEST(TestUnpack, UnpackSve256) { if (!CpuInfo::GetInstance()->IsSupported(CpuInfo::SVE256)) {
