pitrou commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118045776
##########
cpp/cmake_modules/DefineOptions.cmake:
##########
@@ -191,6 +191,9 @@ takes precedence over ccache if a storage backend is
configured" ON)
"SSE4_2"
"AVX2"
"AVX512"
+ "SVE128" # fixed size SVE
+ "SVE256" # "
+ "SVE512" # "
Review Comment:
Does it make sense to have separate per-bitwidth values here, or would "SVE"
be sufficient?
This option merely describes what should be included at compile-time, but
dynamic dispatch will still happen anyway (and the environment varibale
`ARROW_USER_SIMD_LEVEL` can be used to select the SIMD level at runtime).
##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -134,7 +134,29 @@ elseif(ARROW_CPU_FLAG STREQUAL "ppc")
elseif(ARROW_CPU_FLAG STREQUAL "aarch64")
# Arm64 compiler flags, gcc/clang only
set(ARROW_ARMV8_MARCH "armv8-a")
- check_cxx_compiler_flag("-march=${ARROW_ARMV8_MARCH}+sve" CXX_SUPPORTS_SVE)
+ set(ARROW_SVE_FLAGS "-march=${ARROW_ARMV8_MARCH}+sve")
+ set(ARROW_SVE128_FLAGS "${ARROW_SVE_FLAGS}" "-msve-vector-bits=128")
+ set(ARROW_SVE256_FLAGS "${ARROW_SVE_FLAGS}" "-msve-vector-bits=256")
+ set(ARROW_SVE512_FLAGS "${ARROW_SVE_FLAGS}" "-msve-vector-bits=512")
+ if(APPLE)
+ # Clang on MacOS may support SVE but it is not tested anywhere, especially
Review Comment:
Apple CPUs do not support SVE, so I doubt they care about it currently.
##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -134,7 +134,29 @@ elseif(ARROW_CPU_FLAG STREQUAL "ppc")
elseif(ARROW_CPU_FLAG STREQUAL "aarch64")
# Arm64 compiler flags, gcc/clang only
set(ARROW_ARMV8_MARCH "armv8-a")
Review Comment:
Can we add support for MSVC? We have a Windows ARM64 C++ build.
##########
cpp/cmake_modules/DefineOptions.cmake:
##########
@@ -191,6 +191,9 @@ takes precedence over ccache if a storage backend is
configured" ON)
"SSE4_2"
"AVX2"
"AVX512"
+ "SVE128" # fixed size SVE
Review Comment:
Do we want to add "NEON" above or are we ok with mandating it anyway?
##########
cpp/src/arrow/util/bpacking_test.cc:
##########
@@ -349,6 +349,72 @@ TEST_P(TestUnpack, Unpack32Neon) {
this->TestAll(&bpacking::unpack_neon<uint32_t
TEST_P(TestUnpack, Unpack64Neon) {
this->TestAll(&bpacking::unpack_neon<uint64_t>); }
#endif
+#if defined(ARROW_HAVE_RUNTIME_SVE128)
Review Comment:
Copying/pasting all these is starting to get unwiedly, is there an easy way
to factor things out and reduce duplication?
##########
cpp/src/arrow/util/bpacking_simd_128_alt.cc:
##########
@@ -0,0 +1,51 @@
+// 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)
Review Comment:
Same here: should we error out otherwise?
##########
cpp/cmake_modules/DefineOptions.cmake:
##########
@@ -191,6 +191,9 @@ takes precedence over ccache if a storage backend is
configured" ON)
"SSE4_2"
"AVX2"
"AVX512"
+ "SVE128" # fixed size SVE
+ "SVE256" # "
+ "SVE512" # "
Review Comment:
@cyb70289 What do you think?
##########
cpp/src/arrow/util/dispatch_internal.h:
##########
@@ -106,11 +108,16 @@ class DynamicDispatch {
return cpu_info->IsSupported(CpuInfo::AVX2);
case DispatchLevel::AVX512:
return cpu_info->IsSupported(CpuInfo::AVX512);
+ case DispatchLevel::NEON:
+ return cpu_info->IsSupported(CpuInfo::ASIMD);
+ case DispatchLevel::SVE256:
Review Comment:
Why no SVE128 here?
##########
cpp/src/arrow/util/bpacking_simd_internal.h:
##########
@@ -25,68 +25,90 @@
namespace arrow::internal::bpacking {
#if defined(ARROW_HAVE_NEON)
+# define UNPACK_ARCH128 unpack_neon
+#elif defined(ARROW_HAVE_SSE4_2)
+# define UNPACK_ARCH128 unpack_sse4_2
+#endif
Review Comment:
Relying on `ARROW_HAVE_NEON` etc. is why we need the "128 alt" case, right?
Perhaps we can also depend on which target the file is being compiled for.
For example we could have:
```cmake
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}"
COMPILE_DEFINITIONS
"ARROW_COMPILING_FOR_SVE128")
endif()
endmacro()
```
and then:
```c++
#if defined(ARROW_COMPILING_FOR_SVE128)
# define UNPACK_ARCH128 unpack_sve128
#elif defined(ARROW_HAVE_NEON)
# define UNPACK_ARCH128 unpack_neon
#elif defined(ARROW_HAVE_SSE4_2)
# define UNPACK_ARCH128 unpack_sse4_2
#endif
```
##########
cpp/src/arrow/util/bpacking.cc:
##########
@@ -33,16 +33,26 @@ struct UnpackDynamicFunction {
static constexpr auto implementations() {
return std::array{
+ // x86 implementations
#if defined(ARROW_HAVE_SSE4_2)
Implementation{DispatchLevel::NONE, &bpacking::unpack_sse4_2<Uint>},
-#else
- Implementation{DispatchLevel::NONE, &bpacking::unpack_scalar<Uint>},
-#endif
-#if defined(ARROW_HAVE_RUNTIME_AVX2)
+# if defined(ARROW_HAVE_RUNTIME_AVX2)
Implementation{DispatchLevel::AVX2, &bpacking::unpack_avx2<Uint>},
-#endif
-#if defined(ARROW_HAVE_RUNTIME_AVX512)
+# endif
+# if defined(ARROW_HAVE_RUNTIME_AVX512)
Implementation{DispatchLevel::AVX512, &bpacking::unpack_avx512<Uint>},
+# endif
+
+ // ARM implementations
+#elif defined(ARROW_HAVE_NEON)
+ Implementation{DispatchLevel::NONE, &bpacking::unpack_neon<Uint>},
+# if defined(ARROW_HAVE_RUNTIME_SVE256)
Review Comment:
Why no SVE128 here? Does it turn out not useful?
##########
cpp/src/arrow/util/bpacking_simd_128.cc:
##########
@@ -17,8 +17,10 @@
#if defined(ARROW_HAVE_NEON)
# define UNPACK_PLATFORM unpack_neon
+# define KERNEL_PLATFORM KernelNeon
#elif defined(ARROW_HAVE_SSE4_2)
# define UNPACK_PLATFORM unpack_sse4_2
+# define KERNEL_PLATFORM KernelSse42
#endif
#if defined(UNPACK_PLATFORM)
Review Comment:
Should we error out if this is not defined? This file should ideally only be
compiled with the right compiler args.
##########
cpp/src/arrow/util/bpacking_simd_128.cc:
##########
@@ -17,8 +17,10 @@
#if defined(ARROW_HAVE_NEON)
# define UNPACK_PLATFORM unpack_neon
Review Comment:
Can we just include `bpacking_simd_internal.h` and reuse the
`UNPACK_ARCH128` macro?
##########
cpp/src/arrow/util/bpacking_simd_128_alt.cc:
##########
@@ -0,0 +1,51 @@
+// 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
Review Comment:
Hmm, can we reconcile this file and `bpacking_simd_128.cc`?
##########
cpp/src/arrow/util/bpacking_simd_128.cc:
##########
@@ -30,11 +32,11 @@
namespace arrow::internal::bpacking {
template <typename UnpackedUint, int kPackedBitSize>
-using Simd128Kernel = Kernel<UnpackedUint, kPackedBitSize, 128>;
+using KERNEL_PLATFORM = Kernel<UnpackedUint, kPackedBitSize,
xsimd::default_arch>;
Review Comment:
I'm not sure why you need to macro this, it's used just once in the function
below. You can move the `using` declaration with a non-macro name inside the
function, then we don't need `KERNEL_PLATFORM`.
##########
cpp/src/arrow/util/cpu_info.h:
##########
@@ -56,6 +56,10 @@ class ARROW_EXPORT CpuInfo {
/// Arm features
static constexpr int64_t ASIMD = (1LL << 32);
+ static constexpr int64_t SVE = (1LL << 33);
+ static constexpr int64_t SVE128 = (1LL << 36);
+ static constexpr int64_t SVE256 = (1LL << 34);
+ static constexpr int64_t SVE512 = (1LL << 35);
Review Comment:
Why do we have both sized and unsized SVE here? I don't think unsized SVE
applies to us, does it?
cc @cyb70289 for opinions
##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -134,7 +134,29 @@ elseif(ARROW_CPU_FLAG STREQUAL "ppc")
elseif(ARROW_CPU_FLAG STREQUAL "aarch64")
# Arm64 compiler flags, gcc/clang only
set(ARROW_ARMV8_MARCH "armv8-a")
Review Comment:
Scratch that, SVE support for MSVC is so recent that it hasn't dried yet:
https://devblogs.microsoft.com/cppblog/msvc-build-tools-version-14-51-release-candidate-now-available/#arm64-&-sve
--
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]
