You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "AntoinePrv (via GitHub)" <gi...@apache.org> on 2026/04/15 09:39:41 UTC
[PR] Sve dynamic dispatch [arrow]
AntoinePrv opened a new pull request, #49756:
URL: https://github.com/apache/arrow/pull/49756
### Rationale for this change
Just like we dynamically dispatch to AVX2 on x86 CPUs, we want to dynamically dispatch to more advanced SIMD extension on ARM64 chips.
### What changes are included in this PR?
- A new macro to enable selecting the runtime SVE version
- Detection of the ARM64 CPU features available at runtime
- Adding SVE to the dynamic dispatch for bit unpacking algorithms.
### Are these changes tested?
### Are there any user-facing changes?
No.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] Sve dynamic dispatch [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4250960421
:warning: GitHub issue #47769 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4311938388
That is interesting, I was also investigating a `std::memcpy` not inlined in the epilogue.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4334022506
Revision: 5008b13a98a0e0aab869cfdb14003fab7ad3b11f
Submitted crossbow builds: [ursacomputing/crossbow @ actions-5c92403d15](https://github.com/ursacomputing/crossbow/branches/all?query=actions-5c92403d15)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/25045158708/job/73358205475)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/25045159949/job/73358209955)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/25045159562/job/73358208478)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/25045158355/job/73358204190)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/25045158335/job/73358204137)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/25045160147/job/73358210656)|
|test-debian-13-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/25045159407/job/73358207883)|
|test-debian-13-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/25045159491/job/73358208099)|
|test-debian-experimental-cpp-gcc-15|[](https://github.com/ursacomputing/crossbow/actions/runs/25045160156/job/73358210681)|
|test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/25045158130/job/73358203698)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/25045158483/job/73358204882)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/25045158606/job/73358205000)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/25045159134/job/73358207304)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/25045159458/job/73358208619)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/25045160459/job/73358211649)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/25045159400/job/73358207917)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/25045158156/job/73358203935)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/25045160047/job/73358210166)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/25045158142/job/73358203944)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/25045158974/job/73358206639)|
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118848582
##########
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:
**TODO**
A mistake (benchmarks actually run the functions directly without going through the dynamic dispatch).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118864055
##########
cpp/src/arrow/util/bpacking_simd_128.cc:
##########
@@ -17,8 +17,10 @@
#if defined(ARROW_HAVE_NEON)
# define UNPACK_PLATFORM unpack_neon
Review Comment:
Possibly, though I thought it best that the macro is `#undef` at the end of the header (making it useless here).
We can make it more explicit (`ARROW_BPACKING_UNPACK_ARCH128`) an not undefining it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118828375
##########
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:
Unless we're targeting 32bit ARM or embedded ARM, it should always be a given.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4334069816
(opened https://github.com/apache/arrow/issues/49882 for Neon on Windows)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4305829188
@ursabot please benchmark
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] Sve dynamic dispatch [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4250957161
<!--
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.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4325189519
Bad manipulation...
I pushed a commit with more aggressive inlining strategy. All SIMD code seems to be doing better wrt to scala, but the previous conclusions remain: `sve128` is still better than `Neon` and sometimes `Scalar` is the best.
The fix was on the pro/epilogue so this should also benefit x86.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4296366050
Something isn't quite right on [ARM64 Ubuntu](https://github.com/apache/arrow/actions/runs/24768770758/job/72469479821?pr=49756#step:8:982) and [ARM64 macOS](https://github.com/apache/arrow/actions/runs/24768770758/job/72469479837?pr=49756#step:12:639). `-march=armv8-a+sve` is added to the default compiler flags even though we have `ARROW_SIMD_LEVEL=NEON`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3129437510
##########
cpp/src/arrow/util/bpacking.cc:
##########
@@ -52,12 +65,14 @@ struct UnpackDynamicFunction {
template <typename Uint>
void unpack(const uint8_t* in, Uint* out, const UnpackOptions& opts) {
-#if defined(ARROW_HAVE_NEON)
- return bpacking::unpack_neon(in, out, opts);
-#else
- static DynamicDispatch<UnpackDynamicFunction<Uint> > dispatch;
- return dispatch.func(in, out, opts);
-#endif
+ auto constexpr kImplementations = UnpackDynamicFunction<Uint>::implementations();
+ if constexpr (kImplementations.size() == 1) {
Review Comment:
It is worth it to avoid additional `#ifdef`, for instance on Macos there is only neon and no SVE (no need to dyn dispatch).
Previously we'd exclude the Neon version from the dynamic dispatch and go `#ifdef ARROW_HAVE_NEON` then go straight to Neon implementation.
> At worse, this could be added generically to `DynamicDispatch` instead. But I doubt it's worth it.
Actually done in GH-49840 so either way here (we'd need to adapt the PR that is not merged first).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3129377807
##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -541,7 +563,7 @@ if(ARROW_CPU_FLAG STREQUAL "aarch64")
add_definitions(-DARROW_HAVE_SVE_SIZELESS)
endif()
endif()
- set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -march=${ARROW_ARMV8_MARCH}")
+ set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} ${ARROW_SVE_FLAGS}")
Review Comment:
Good catch! This is the static case, but reworking the flag lead to this bug.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4308578845
Thanks for your patience. Conbench analyzed the 2 benchmarking runs that have been run so far on PR commit cbf526fd1e0b11305277ab7a4eee7129ac2c9670.
There were 14 benchmark results indicating a performance regression:
- Pull Request Run on `amd64-c6a-4xlarge-linux` at [2026-04-23 16:51:19Z](https://conbench.arrow-dev.org/compare/runs/3068bdc3ee4445379d7e0ef65a769952...bf806f133f21443b839f81db5db46d0b/)
- [`BM_UnpackUint16` (C++) with params=Avx2Unaligned/13/256, source=cpp-micro, suite=arrow-bpacking-benchmark](https://conbench.arrow-dev.org/compare/benchmarks/069e9f3ebcf478088000ffe1a4a57d68...069ea4e9ec8e7eb08000d8eae1e2581b)
- [`BM_UnpackUint16` (C++) with params=DynamicUnaligned/13/64, source=cpp-micro, suite=arrow-bpacking-benchmark](https://conbench.arrow-dev.org/compare/benchmarks/069e9f3ed9567a1c8000f7c35ed9f504...069ea4ea03727abd80004bae3fd5302f)
- and 12 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/72786863409) has more details.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "cyb70289 (via GitHub)" <gi...@apache.org>.
cyb70289 commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4310376121
> But it's impressive that SVE128 is always significantly better than NEON. That's rather good news, given that most SVE implementations have 128-bit vectors. @cyb70289
Interesting. It should not happen if both using equivalent simd operations.
I tested one case `BM_UnpackBool/{Neon,Sve128}Unaligned/1/32` on an Neoverse N2 server, SVE shows double performance than Neon. But from profile result, looks Neon code does not inline frequently called functions like `load_val_as` and introduces high overhead.
**Benchmark**
```
BM_UnpackBool/NeonUnaligned/1/32 11.1 ns 11.0 ns 63329847 items_per_second=2.89667G/s
BM_UnpackBool/Sve128Unaligned/1/32 7.02 ns 7.02 ns 99743767 items_per_second=4.55851G/s
```
**Neon hotspot shows load_val_as is not inlined**
```
+ 45.20% arrow-bpacking- libarrow.so.2500.0.0 [.] void arrow::internal::bpacking::unpack_width<1, arrow::internal::bpacking::KernelNeon, bool>(unsigned char const
+ 31.03% arrow-bpacking- libarrow.so.2500.0.0 [.] xsimd::batch<unsigned char, xsimd::neon64> arrow::internal::bpacking::load_val_as<unsigned int, xsimd::neon64>(u
+ 7.08% arrow-bpacking- libarrow.so.2500.0.0 [.] void arrow::internal::bpacking::MediumKernel<arrow::internal::bpacking::KernelTraits<bool, 1, xsimd::neon64>, ar
+ 6.38% arrow-bpacking- libarrow.so.2500.0.0 [.] void arrow::internal::bpacking::MediumKernel<arrow::internal::bpacking::KernelTraits<bool, 1, xsimd::neon64>, ar
+ 2.63% arrow-bpacking- libarrow.so.2500.0.0 [.] void arrow::internal::bpacking::unpack_neon<bool>(unsigned char const*, bool*, arrow::internal::UnpackOptions co
+ 1.95% arrow-bpacking- arrow-bpacking-benchmark [.] arrow::internal::(anonymous namespace)::BM_UnpackBool(benchmark::State&, bool, void (*)(unsigned char const*, bo
+ 1.82% arrow-bpacking- libarrow.so.2500.0.0 [.] xsimd::batch<unsigned char, xsimd::neon64> arrow::internal::bpacking::load_val_as<unsigned int, xsimd::neon64>(u
+ 1.37% arrow-bpacking- libarrow.so.2500.0.0 [.] void arrow::internal::bpacking::unpack_width<1, arrow::internal::bpacking::KernelNeon, bool>(unsigned char const
```
**No such issue in sve128 code path**
```
+ 89.89% arrow-bpacking- libarrow.so.2500.0.0 [.] void arrow::internal::bpacking::unpack_width<1, arrow::int◆
+ 4.18% arrow-bpacking- libarrow.so.2500.0.0 [.] void arrow::internal::bpacking::unpack_sve128<bool>(unsign▒
+ 3.11% arrow-bpacking- arrow-bpacking-benchmark [.] arrow::internal::(anonymous namespace)::BM_UnpackBool(benc▒
+ 1.47% arrow-bpacking- libarrow.so.2500.0.0 [.] void arrow::internal::bpacking::unpack_width<1, arrow::int▒
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv closed pull request #49756: GH-47769: [C++] SVE dynamic dispatch
URL: https://github.com/apache/arrow/pull/49756
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4325647823
Benchmark runs are scheduled for commit e11d5ee059a142b543d37de9e46bd93c97440303. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4290205445
@pitrou I definitely agree with the duplication of the different files, it's pretty tedious.
I think it will too large this PR, but we should definitely think of something, including providing some CMake utilities in xsimd.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118821844
##########
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:
Since we do not use sizeless SVE, we need to rebuild the SIMD code for each size, so it can be desirable to choose how big the binary will be.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118870996
##########
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:
Not in this file I believe. I thing there are Arrow builds where SIMD is deactivated and this file is not excluded from the build tree (as opposed to the 256/512 versions).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4326095230
It seems that `__forceinline` on MSVC implies `inline` and so the compiler complains when you combine them:
> D:\a\arrow\arrow\cpp\src\arrow/util/bpacking_dispatch_internal.h(63): error C2220: the following warning is treated as an error
> D:\a\arrow\arrow\cpp\src\arrow/util/bpacking_dispatch_internal.h(63): warning C4141: 'inline': used more than once
I think the solution is to add `inline` to the `ARROW_FORCE_INLINE` definition on non-MSVC compilers.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4325647181
@ursabot please benchmark lang=C++
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4326343963
Thanks for your patience. Conbench analyzed the 3 benchmarking runs that have been run so far on PR commit e11d5ee059a142b543d37de9e46bd93c97440303.
There were 27 benchmark results indicating a performance regression:
- Pull Request Run on `amd64-m5-4xlarge-linux` at [2026-04-27 10:26:15Z](https://conbench.arrow-dev.org/compare/runs/7a626dc906944475893626b866e730a1...6f3dd1b16392475f8d2385399d629a2b/)
- [`BM_UnpackUint32` (C++) with params=Sse42Unaligned/2/32, source=cpp-micro, suite=arrow-bpacking-benchmark](https://conbench.arrow-dev.org/compare/benchmarks/069eb371ce297c088000202d86ab0210...069ef3a3cfed791f8000562886fc086d)
- [`BM_UnpackUint32` (C++) with params=Sse42Unaligned/8/256, source=cpp-micro, suite=arrow-bpacking-benchmark](https://conbench.arrow-dev.org/compare/benchmarks/069eb371f7e9720b8000ccd789dec940...069ef3a3d3557e5f8000b38ef2e39f8d)
- and 25 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/73175528490) has more details.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3129441011
##########
cpp/src/arrow/util/bpacking_benchmark.cc:
##########
@@ -107,10 +107,10 @@ void BM_Unpack(benchmark::State& state, bool aligned, UnpackFunc<Int> unpack, bo
// will not emit runs larger than 512 (though other implementation might), so we biased
// the benchmarks towards a rather small scale.
static const auto kNumValuesRange = benchmark::CreateRange(32, 512, 2);
-constexpr std::initializer_list<int64_t> kBitWidths8 = {1, 2, 8};
-constexpr std::initializer_list<int64_t> kBitWidths16 = {1, 2, 8, 13};
-constexpr std::initializer_list<int64_t> kBitWidths32 = {1, 2, 8, 20};
-constexpr std::initializer_list<int64_t> kBitWidths64 = {1, 2, 8, 20, 47};
+static const auto kBitWidths8 = benchmark::CreateDenseRange(0, 8, 1);
+static const auto kBitWidths16 = benchmark::CreateDenseRange(0, 16, 1);
+static const auto kBitWidths32 = benchmark::CreateDenseRange(0, 32, 1);
+static const auto kBitWidths64 = benchmark::CreateDenseRange(0, 64, 1);
Review Comment:
Sorry, did not mean to push this commit, this was for local benchmarking.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118842014
##########
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:
Exactly, this condition disables it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118895513
##########
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:
Unfortunately we cannot because this alias needs to be parameterized by types (this is a requirement from `unpack_jump`) and this is not possible inside a function.
As for the different names for `KERNEL_PLATFORM`, this is a better safe than sorry ODR violation prevention. Although perhaps we could do better with inline namespaces.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3131318296
##########
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:
You mean with a macro?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4305497305
@pitrou here are some benchmarks in the [0, 500] num of integer to unpack that Arrow is operating over.
- SVE128: [linux-arm64-graviton4.pdf](https://github.com/user-attachments/files/27016293/linux-arm64-graviton4.pdf)
- SVE256: [linux-arm64-graviton3.pdf](https://github.com/user-attachments/files/27016255/linux-arm64-graviton3.pdf)
As before, we can sometimes be much penalized by small sizes.
Surprisingly (annoyingly?) there are some bit width numbers (e.g. 3, 5, 6, 7 on SVE256) where the world is upside down: scalar does best, then Neon, then SVE (in the [0, 500] range, it does not hold for larger input buffers).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4305821451
> Surprisingly (annoyingly?) there are some bit width numbers (e.g. 3, 5, 6, 7 on SVE256) where the world is upside down: scalar does best, then Neon, then SVE (in the [0, 500] range, it does not hold for larger input buffers).
Perhaps because of larger vectors and a slow epilogue?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4327079107
Our ARM benchmarking platform, which as NEON but not SVE (I think it's a Graviton 2 machine), shows very nice speedups on some Parquet reading/decoding benchmarks. :tada:
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3124084383
##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -541,7 +563,7 @@ if(ARROW_CPU_FLAG STREQUAL "aarch64")
add_definitions(-DARROW_HAVE_SVE_SIZELESS)
endif()
endif()
- set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -march=${ARROW_ARMV8_MARCH}")
+ set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} ${ARROW_SVE_FLAGS}")
Review Comment:
I think this is the source of the problem: SVE should only be enabled when compiling the dynamically-dispatched variants. Otherwise this might generate SVE instructions in the rest of the codebase.
##########
cpp/cmake_modules/SetupCxxFlags.cmake:
##########
@@ -528,8 +552,6 @@ if(ARROW_CPU_FLAG STREQUAL "aarch64")
if(NOT CXX_SUPPORTS_SVE)
message(FATAL_ERROR "SVE required but compiler doesn't support it.")
endif()
- # -march=armv8-a+sve
- set(ARROW_ARMV8_MARCH "${ARROW_ARMV8_MARCH}+sve")
Review Comment:
This was ok: it was enabling SVE unconditionally only if `ARROW_SIMD_LEVEL` was set to 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118848582
##########
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:
A mistake (benchmarks actually run the functions directly without going through the dynamic dispatch).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3124112195
##########
cpp/src/arrow/util/bpacking.cc:
##########
@@ -52,12 +65,14 @@ struct UnpackDynamicFunction {
template <typename Uint>
void unpack(const uint8_t* in, Uint* out, const UnpackOptions& opts) {
-#if defined(ARROW_HAVE_NEON)
- return bpacking::unpack_neon(in, out, opts);
-#else
- static DynamicDispatch<UnpackDynamicFunction<Uint> > dispatch;
- return dispatch.func(in, out, opts);
-#endif
+ auto constexpr kImplementations = UnpackDynamicFunction<Uint>::implementations();
+ if constexpr (kImplementations.size() == 1) {
Review Comment:
Is this condition actually useful? I guess it's a shortcut, but it's not obvious that it applies to common cases (x86 or ARM with default SIMD options).
At worse, this could be added generically to `DynamicDispatch` instead. But I doubt it's worth it.
##########
cpp/src/arrow/util/bpacking_benchmark.cc:
##########
@@ -107,10 +107,10 @@ void BM_Unpack(benchmark::State& state, bool aligned, UnpackFunc<Int> unpack, bo
// will not emit runs larger than 512 (though other implementation might), so we biased
// the benchmarks towards a rather small scale.
static const auto kNumValuesRange = benchmark::CreateRange(32, 512, 2);
-constexpr std::initializer_list<int64_t> kBitWidths8 = {1, 2, 8};
-constexpr std::initializer_list<int64_t> kBitWidths16 = {1, 2, 8, 13};
-constexpr std::initializer_list<int64_t> kBitWidths32 = {1, 2, 8, 20};
-constexpr std::initializer_list<int64_t> kBitWidths64 = {1, 2, 8, 20, 47};
+static const auto kBitWidths8 = benchmark::CreateDenseRange(0, 8, 1);
+static const auto kBitWidths16 = benchmark::CreateDenseRange(0, 16, 1);
+static const auto kBitWidths32 = benchmark::CreateDenseRange(0, 32, 1);
+static const auto kBitWidths64 = benchmark::CreateDenseRange(0, 64, 1);
Review Comment:
This will create a large number of nano-benchmarks, do really want this?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118913740
##########
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:
Right I don't think we need it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118915854
##########
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:
*TODO*
Omission
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4305827596
But it's impressive that SVE128 is always significantly better than NEON. That's rather good news, given that most SVE implementations have 128-bit vectors. @cyb70289
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4334001564
@github-actions crossbow submit -g cpp
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4337633096
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit d1a8cf4133e94941850d1a709f5263d82d1f45ed.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/73438732804) has more details. It also includes information about 18 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4333993106
Interestingly, Neon doesn't seem enabled on our ARM64 Windows builds.
I'll open a separate issue in case an interested contributor comes up.
https://github.com/apache/arrow/actions/runs/25043252865/job/73351674163?pr=49756#step:11:377
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #49756:
URL: https://github.com/apache/arrow/pull/49756
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "cyb70289 (via GitHub)" <gi...@apache.org>.
cyb70289 commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4326326915
**Just for reference**
Did a quick poke with **AI coding agent**. It analyzed the reason why Neon code is not inlined and proposed a fix to xsimd: [neon-bitcast-inline.patch](https://github.com/user-attachments/files/27121829/neon-bitcast-inline.patch)
Unit test passed. Neon code is slightly faster than SVE128, matches expectation. I only tested one case.
```
# neon
BM_UnpackBool/NeonUnaligned/1/32 6.56 ns 6.56 ns 107048724 items_per_second=4.87937G/s
# sve128
BM_UnpackBool/Sve128Unaligned/1/32 7.06 ns 7.06 ns 99251620 items_per_second=4.53545G/s
```
I suspected xsimd bitcast Neon code may be too complicated for compiler to inline (maybe related to my old PR to fix an [issue](https://github.com/xtensor-stack/xsimd/issues/573), but I forgot details).
Debug report from coding agent (I haven't read it carefully): [findings.md](https://github.com/user-attachments/files/27122154/findings.md)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118836307
##########
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:
I am not aware of a mechanism to detect SVE capabilities at runtime outside of Linux.
+ I believe we have a few things to fix in xsimd for SVE on Windows ARM.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3122597457
##########
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:
> The issue is we need the file compiled twice in ARM (Neon + sve128).
> I think it is not possible directly in CMake.
The easy workaround is to have the same `.h` file included in two different stub `.cc` files.
For example have `bpacking_simd128_internal.h` included by both `bpacking_neon.cc` and `bpacking_sve128.cc`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3131323995
##########
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:
Yes!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3129619860
##########
cpp/src/arrow/util/bpacking.cc:
##########
@@ -52,12 +65,14 @@ struct UnpackDynamicFunction {
template <typename Uint>
void unpack(const uint8_t* in, Uint* out, const UnpackOptions& opts) {
-#if defined(ARROW_HAVE_NEON)
- return bpacking::unpack_neon(in, out, opts);
-#else
- static DynamicDispatch<UnpackDynamicFunction<Uint> > dispatch;
- return dispatch.func(in, out, opts);
-#endif
+ auto constexpr kImplementations = UnpackDynamicFunction<Uint>::implementations();
+ if constexpr (kImplementations.size() == 1) {
Review Comment:
> Actually done in [GH-49840](https://github.com/apache/arrow/pull/49840) so either way here
That PR might prove difficult to adapt for all the lousy compilers we have to support, so I'd rather focus on this one first :)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118836307
##########
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:
I am not aware of a mechanism to detect SVE capabilities at runtime outside of Linux.
\+ I believe we have a few things to fix in xsimd for SVE on Windows ARM.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3122553088
##########
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:
The issue is we need the file compiled twice in ARM (Neon + sve128).
I think it is not possible directly in CMake. The solution is copy to build tree then compile each with different flags.
Is a CMake-only solution satisfactory?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4327392721
> I suspected xsimd bitcast Neon code may be too complicated for compiler to inline (maybe related to my old PR to fix an [issue](https://github.com/xtensor-stack/xsimd/issues/573), but I forgot details).
@cyb70289 for unrelated reasons (building WIN ARM64), I ended up flattening this implementation as well in https://github.com/xtensor-stack/xsimd/pull/1317. We should be merging it and releasing soon after so we'll see how it performs.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118905276
##########
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:
Unfortunately not. On ARM we now need to compile one 128 bit version with Neon, and one 128 bit version with SVE128. Different compiler flags.
I have been thinking of having one single flags had having cmake duplicate it depending on configuration but I beleive we need to support Meson too, right?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118907355
##########
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:
**TODO**
Here we can
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #49756:
URL: https://github.com/apache/arrow/pull/49756#discussion_r3118895513
##########
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:
Unfortunately we cannot because this alias needs to be parameterized by types (this is a requirement from unpack_jump) and this is not possible inside a function.
As for the different names for `KERNEL_PLATFORM`, this is a better safe than sorry ODR violation prevention. Although perhaps we could do better with inline namespaces.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4304971951
@AntoinePrv Is it possible to run some ARM benchmarks and paste the results somewhere once you're satisfied with the PR?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4305829744
Benchmark runs are scheduled for commit cbf526fd1e0b11305277ab7a4eee7129ac2c9670. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47769: [C++] SVE dynamic dispatch [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #49756:
URL: https://github.com/apache/arrow/pull/49756#issuecomment-4305851067
Silly me, I started the continuous benchmarking suite but our ARM platform there (arm64-t4g-2xlarge) uses Graviton 2 CPUs which don't support 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org