Re: [PR] GH-45331: Use xsimd for CPU feature detection [arrow]
pitrou commented on code in PR #49940:
URL: https://github.com/apache/arrow/pull/49940#discussion_r3381015294
##
cpp/src/arrow/CMakeLists.txt:
##
@@ -855,15 +851,13 @@ if(ARROW_COMPUTE)
list(APPEND ARROW_COMPUTE_SHARED_INSTALL_INTERFACE_LIBS Arrow::arrow_shared)
list(APPEND ARROW_COMPUTE_STATIC_LINK_LIBS arrow_static)
list(APPEND ARROW_COMPUTE_SHARED_LINK_LIBS arrow_shared)
+ list(APPEND ARROW_COMPUTE_STATIC_LINK_LIBS ${ARROW_XSIMD})
+ list(APPEND ARROW_COMPUTE_SHARED_PRIVATE_LINK_LIBS ${ARROW_XSIMD})
Review Comment:
I don't see xsimd in use anywhere in `arrow/compute`, do you remember why
this was required @raulcd ?
Edit: perhaps because of `src/arrow/util/utf8_internal.h`? It's a pity this
indirect dependency isn't picked up automatically.
--
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]
Re: [PR] GH-45331: Use xsimd for CPU feature detection [arrow]
github-actions[bot] commented on PR #49940: URL: https://github.com/apache/arrow/pull/49940#issuecomment-4633057133 Revision: feda2a5a75c7cf4affc2f55ba000f934685ad912 Submitted crossbow builds: [ursacomputing/crossbow @ actions-eced990723](https://github.com/ursacomputing/crossbow/branches/all?query=actions-eced990723) |Task|Status| ||--| |example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/27023677306/job/79758030769)| |example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/27023677893/job/79758033079)| |example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/27023676792/job/79758029041)| |test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/27023678522/job/79758036493)| |test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/27023677180/job/79758030120)| |test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/27023676761/job/79758029430)| |test-debian-13-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/27023677077/job/79758029881)| |test-debian-13-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/27023678623/job/79758036374)| |test-debian-experimental-cpp-gcc-15|[](https://github.com/ursacomputing/crossbow/actions/runs/27023678669/job/79758036865)| |test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/27023677464/job/79758031233)| |test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/27023678120/job/79758033792)| |test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/27023676614/job/79758028901)| |test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/27023676604/job/79758028756)| |test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/27023676946/job/79758029500)| |test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/27023678403/job/79758034467)| |test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/27023678052/job/79758034386)| |test-ubuntu-24.04-cpp-gcc-13-bundled|[ {
return static_cast(size);
}
-// Helper function to parse for hardware flags from /proc/cpuinfo
-// values contains a list of space-separated flags. check to see if the flags
we
-// care about are present.
-// Returns a bitmap of flags.
-int64_t LinuxParseCpuFlags(const std::string& values) {
-# if defined(CPUINFO_ARCH_X86) || defined(CPUINFO_ARCH_ARM)
- const struct {
-std::string name;
-int64_t flag;
- } flag_mappings[] = {
-#if defined(CPUINFO_ARCH_X86)
- {"ssse3", CpuInfo::SSSE3}, {"sse4_1", CpuInfo::SSE4_1},
- {"sse4_2", CpuInfo::SSE4_2}, {"popcnt", CpuInfo::POPCNT},
- {"avx", CpuInfo::AVX}, {"avx2", CpuInfo::AVX2},
- {"avx512f", CpuInfo::AVX512F}, {"avx512cd", CpuInfo::AVX512CD},
- {"avx512vl", CpuInfo::AVX512VL}, {"avx512dq", CpuInfo::AVX512DQ},
- {"avx512bw", CpuInfo::AVX512BW}, {"bmi1", CpuInfo::BMI1},
- {"bmi2", CpuInfo::BMI2},
-#elif defined(CPUINFO_ARCH_ARM)
- {"asimd", CpuInfo::ASIMD},
-#endif
- };
- const int64_t num_flags = sizeof(flag_mappings) / sizeof(flag_mappings[0]);
-
- int64_t flags = 0;
- for (int i = 0; i < num_flags; ++i) {
-if (values.find(flag_mappings[i].name) != std::string::npos) {
- flags |= flag_mappings[i].flag;
-}
- }
- return flags;
-# else
- return 0;
-# endif
-}
-
-void OsRetrieveCacheSize(std::array* cache_sizes) {
- for (int i = 0; i < kCacheLevels; ++i) {
+template
+void OsRetrieveCacheSize(std::array* cache_sizes) {
+ static_assert(N <= 3);
+ for (int i = 0; i < static_cast(N); ++i) {
const int64_t cache_size = LinuxGetCacheSize(i);
Review Comment:
Hard-coding `static_assert(N <= 3)` bakes in the current number of cache
levels and will fail to compile if `CpuInfo::CacheLevel` is extended in the
future. Prefer expressing the limit in terms of `CpuInfo`’s cache-level
constant (or deriving the max supported by the Linux helpers), and handle
additional levels gracefully (e.g., leave higher levels at 0 so
`CpuInfo::CacheSize` falls back to defaults).
##
cpp/src/arrow/util/cpu_info.h:
##
@@ -70,16 +66,19 @@ class ARROW_EXPORT CpuInfo {
static const CpuInfo* GetInstance();
/// Returns all the flags for this cpu
- int64_t hardware_flags() const;
+ int64_t hardware_flags() const { return hardware_flags_; }
/// Returns the number of cores (including hyper-threaded) on this machine.
- int num_cores() const;
+ int num_cores() const { return num_cores_ <= 0 ? 1 : num_cores_; }
/// Returns the vendor of the cpu.
- Vendor vendor() const;
+ Vendor vendor() const { return vendor_; }
/// Returns the model name of the cpu (e.g. Intel i7-2600)
- const std::string& model_name() const;
+ std::string_view model_name() const {
+// Unavailable in xsimd at the time of migration and previously unused.
+return "Unknown";
+ }
Review Comment:
`CpuInfo::model_name()` changes from `const std::string&` (and previously
returned an actual detected model name) to `std::string_view` always returning
a string literal. This is a breaking API/ABI change for downstream consumers
and also removes existing functionality. Consider preserving the existing
signature/behavior by storing a `std::string model_name_` member populated
where possible (even if it’s just \"Unknown\" initially), and only add a new
accessor (e.g., `std::string_view model_name_view() const`) if needed.
##
cpp/src/arrow/util/cpu_info.cc:
##
@@ -17,64 +17,107 @@
// From Apache Impala (incubating) as of 2016-01-29.
-#include "arrow/util/cpu_info.h"
-
-#ifdef __APPLE__
-# include
-#endif
-
-#ifndef _MSC_VER
-# include
-#endif
-
-#ifdef _WIN32
-# include
-
-# include "arrow/util/windows_compatibility.h"
-#endif
-
#include
#include
-#include
#include
#include
#include
-#include
-#include
#include
#include
#include
+#include
+
#include "arrow/result.h"
+#include "arrow/util/cpu_info.h"
#include "arrow/util/io_util.h"
#include "arrow/util/logging_internal.h"
#include "arrow/util/string.h"
-#undef CPUINFO_ARCH_X86
-#undef CPUINFO_ARCH_ARM
-#undef CPUINFO_ARCH_PPC
-
-#if defined(__i386) || defined(_M_IX86) || defined(__x86_64__) ||
defined(_M_X64)
-# define CPUINFO_ARCH_X86
-#elif defined(_M_ARM64) || defined(__aarch64__) || defined(__arm64__)
-# define CPUINFO_ARCH_ARM
-#elif defined(__PPC64__) || defined(__PPC64LE__) || defined(__ppc64__) || \
-defined(__powerpc64__)
-# define CPUINFO_ARCH_PPC
+#ifdef __linux__
+# include
#endif
-namespace arrow {
-namespace internal {
+#ifdef __APPLE__
+# include
+#endif
+
+#ifndef _MSC_VER
+# include
+#endif
+
+#ifdef _WIN32
+# include
+
+# include "arrow/util/windows_compatibility.h"
+#endif
+
+namespace arr
Re: [PR] GH-45331: Use xsimd for CPU feature detection [arrow]
AntoinePrv commented on PR #49940: URL: https://github.com/apache/arrow/pull/49940#issuecomment-4632726628 I pushed a fix for emscripten (and other ?), though I think the issue must have been there before. I think the rest must be unreleated. -- 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]
Re: [PR] GH-45331: Use xsimd for CPU feature detection [arrow]
github-actions[bot] commented on PR #49940: URL: https://github.com/apache/arrow/pull/49940#issuecomment-4630981443 Revision: f5cab6044ce80f8aa1c88a98692d4b89dabf2269 Submitted crossbow builds: [ursacomputing/crossbow @ actions-f9e8dca2bd](https://github.com/ursacomputing/crossbow/branches/all?query=actions-f9e8dca2bd) |Task|Status| ||--| |example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/27011615032/job/79716315017)| |example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/27011616782/job/79716320780)| |example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/27011615133/job/79716315103)| |test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/27011615979/job/79716317510)| |test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/27011616347/job/79716319023)| |test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/27011615375/job/79716315870)| |test-debian-13-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/27011616970/job/79716321319)| |test-debian-13-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/27011616685/job/79716320191)| |test-debian-experimental-cpp-gcc-15|[](https://github.com/ursacomputing/crossbow/actions/runs/27011616036/job/79716317690)| |test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/27011616738/job/79716320442)| |test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/27011615487/job/79716315991)| |test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/27011616826/job/79716320880)| |test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/27011616611/job/79716320263)| |test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/27011616540/job/79716319996)| |test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/27011616088/job/79716317926)| |test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/27011615736/job/79716316761)| |test-ubuntu-24.04-cpp-gcc-13-bundled|[ || defined(ARROW_HAVE_SSE4_2)` and we are now catching cases were xsimd is in fact not available. -- 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]
Re: [PR] GH-45331: Use xsimd for CPU feature detection [arrow]
github-actions[bot] commented on PR #49940: URL: https://github.com/apache/arrow/pull/49940#issuecomment-4624466195 Revision: 557d65b74f66016441ed4fdbc884c39a8add2e9d Submitted crossbow builds: [ursacomputing/crossbow @ actions-ee545fa2be](https://github.com/ursacomputing/crossbow/branches/all?query=actions-ee545fa2be) |Task|Status| ||--| |example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/26967328694/job/79573295822)| |example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/26967331398/job/79573305151)| |example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/26967330001/job/79573300446)| |test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/26967328703/job/79573295090)| |test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/26967327859/job/79573291754)| |test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/26967328063/job/79573293304)| |test-debian-13-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967326874/job/79573287857)| |test-debian-13-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/26967327019/job/79573288250)| |test-debian-experimental-cpp-gcc-15|[](https://github.com/ursacomputing/crossbow/actions/runs/26967328079/job/79573292268)| |test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/26967328439/job/79573293895)| |test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/26967328965/job/79573296976)| |test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/26967331000/job/79573304000)| |test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/26967328672/job/79573294658)| |test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/26967326651/job/79573287778)| |test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/26967327259/job/79573289891)| |test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/26967329478/job/79573299088)| |test-ubuntu-24.04-cpp-gcc-13-bundled|[ |Task|Status| ||--| |wheel-macos-monterey-cp314-cp314-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967341123/job/79573338145)| |wheel-macos-monterey-cp314-cp314-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967341031/job/79573338023)| |wheel-macos-monterey-cp314-cp314t-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967339218/job/79573330173)| |wheel-macos-monterey-cp314-cp314t-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967340152/job/79573334215)| |wheel-manylinux-2-28-cp314-cp314-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967340621/job/79573337423)| |wheel-manylinux-2-28-cp314-cp314-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967341247/job/79573340111)| |wheel-manylinux-2-28-cp314-cp314t-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967341517/job/79573340304)| |wheel-manylinux-2-28-cp314-cp314t-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967340176/job/79573335345)| |wheel-musllinux-1-2-cp314-cp314-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967340537/job/79573336769)| |wheel-musllinux-1-2-cp314-cp314-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967339499/job/79573331534)| |wheel-musllinux-1-2-cp314-cp314t-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967341966/job/79573340748)| |wheel-musllinux-1-2-cp314-cp314t-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967340159/job/79573334134)| |wheel-windows-cp314-cp314-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967340319/job/79573336053)| |wheel-windows-cp314-cp314t-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/26967338853/job/79573328932)| -- 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]
Re: [PR] GH-45331: Use xsimd for CPU feature detection [arrow]
pitrou commented on PR #49940: URL: https://github.com/apache/arrow/pull/49940#issuecomment-4624445063 @github-actions crossbow submit *wheel*cp314* -- 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]
Re: [PR] GH-45331: Use xsimd for CPU feature detection [arrow]
pitrou commented on PR #49940: URL: https://github.com/apache/arrow/pull/49940#issuecomment-4624443915 @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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] GH-45331: Use xsimd for CPU feature detection [arrow]
AntoinePrv commented on PR #49940: URL: https://github.com/apache/arrow/pull/49940#issuecomment-4613822030 @pitrou this is in pretty good shape ! -- 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]
Re: [PR] GH-45331: Use xsimd for CPU feature detection [arrow]
Copilot commented on code in PR #49940:
URL: https://github.com/apache/arrow/pull/49940#discussion_r3349013509
##
cpp/src/arrow/util/cpu_info.h:
##
@@ -70,16 +66,19 @@ class ARROW_EXPORT CpuInfo {
static const CpuInfo* GetInstance();
/// Returns all the flags for this cpu
- int64_t hardware_flags() const;
+ int64_t hardware_flags() const { return hardware_flags_; }
/// Returns the number of cores (including hyper-threaded) on this machine.
- int num_cores() const;
+ int num_cores() const { return num_cores_ <= 0 ? 1 : num_cores_; }
/// Returns the vendor of the cpu.
- Vendor vendor() const;
+ Vendor vendor() const { return vendor_; }
/// Returns the model name of the cpu (e.g. Intel i7-2600)
- const std::string& model_name() const;
+ std::string_view model_name() const {
+// Unavailable in xsimd at the time of migration and previously unused.
+return "Unknown";
+ }
Review Comment:
This changes the public API/ABI of `CpuInfo::model_name()` from `const
std::string&` to `std::string_view`, and also changes behavior to always return
a literal. Even in `arrow::internal`, `ARROW_EXPORT` suggests this can be
consumed across shared library boundaries. Consider preserving the existing
signature/ABI (e.g., keep returning `const std::string&` backed by a member) or
introducing a new accessor while keeping the old one for compatibility, and
update the comment/doc accordingly.
##
cpp/src/arrow/util/cpu_info.cc:
##
@@ -17,64 +17,107 @@
// From Apache Impala (incubating) as of 2016-01-29.
-#include "arrow/util/cpu_info.h"
-
-#ifdef __APPLE__
-# include
-#endif
-
-#ifndef _MSC_VER
-# include
-#endif
-
-#ifdef _WIN32
-# include
-
-# include "arrow/util/windows_compatibility.h"
-#endif
-
#include
#include
-#include
#include
#include
#include
-#include
-#include
#include
#include
#include
+#include
+
#include "arrow/result.h"
+#include "arrow/util/cpu_info.h"
#include "arrow/util/io_util.h"
#include "arrow/util/logging_internal.h"
#include "arrow/util/string.h"
-#undef CPUINFO_ARCH_X86
-#undef CPUINFO_ARCH_ARM
-#undef CPUINFO_ARCH_PPC
-
-#if defined(__i386) || defined(_M_IX86) || defined(__x86_64__) ||
defined(_M_X64)
-# define CPUINFO_ARCH_X86
-#elif defined(_M_ARM64) || defined(__aarch64__) || defined(__arm64__)
-# define CPUINFO_ARCH_ARM
-#elif defined(__PPC64__) || defined(__PPC64LE__) || defined(__ppc64__) || \
-defined(__powerpc64__)
-# define CPUINFO_ARCH_PPC
+#ifdef __linux__
+# include
#endif
-namespace arrow {
-namespace internal {
+#ifdef __APPLE__
+# include
+#endif
+
+#ifndef _MSC_VER
+# include
+#endif
+
+#ifdef _WIN32
+# include
+
+# include "arrow/util/windows_compatibility.h"
+#endif
+
+namespace arrow::internal {
namespace {
-constexpr int kCacheLevels = static_cast(CpuInfo::CacheLevel::Last) + 1;
+void OsRetrieveCpuInfo(int64_t* hardware_flags, CpuInfo::Vendor* vendor) {
+ const auto cpu = xsimd::cpu_features();
+
+ *hardware_flags |= cpu.popcnt() ? CpuInfo::POPCNT : 0;
+ *hardware_flags |= cpu.bmi1() ? CpuInfo::BMI1 : 0;
+ *hardware_flags |= cpu.bmi2() ? CpuInfo::BMI2 : 0;
+
+ // SSE
+ *hardware_flags |= cpu.ssse3() ? CpuInfo::SSSE3 : 0;
+ *hardware_flags |= cpu.sse4_1() ? CpuInfo::SSE4_1 : 0;
+ *hardware_flags |= cpu.sse4_2() ? CpuInfo::SSE4_2 : 0;
+ // AVX
+ *hardware_flags |= cpu.avx() ? CpuInfo::AVX : 0;
+ *hardware_flags |= cpu.avx2() ? CpuInfo::AVX2 : 0;
+ // AVX 512
+ const bool avx512f = cpu.avx512f();
+ *hardware_flags |= cpu.avx512f() ? CpuInfo::AVX512F : 0;
+ *hardware_flags |= cpu.avx512cd() ? CpuInfo::AVX512CD : 0;
+ *hardware_flags |= cpu.avx512dq() ? CpuInfo::AVX512DQ : 0;
+ *hardware_flags |= cpu.avx512bw() ? CpuInfo::AVX512BW : 0;
+ // TODO(xsimd): Missing in xsimd 14.2.0 but fixed afterwards.
+ // Can be replaced with the following (no `if(avx512f)` required).
+ // *hardware_flags |= cpu.avx512vl() ? CpuInfo::AVX512VL : 0;
+ if (avx512f) {
+const auto cpu_x86 = xsimd::x86_cpu_features_backend_default();
+auto constexpr avx512vl = static_cast(31);
+*hardware_flags |= cpu_x86.leaf7().all_bits_set() ?
CpuInfo::AVX512VL : 0;
+ }
Review Comment:
The AVX512VL detection looks like it’s casting the integer `31` as the bit
*mask* enum value, but cpuid feature enums are commonly defined as `(1u <<
bit_index)`. If `xsimd::x86_cpuid_leaf7::ebx` is a mask enum, `31` would check
the wrong bits and mis-detect AVX512VL. Prefer using the named enum constant
(if available) or the correct mask value for bit 31 (e.g., `1u << 31`) per
xsimd’s cpuid API.
##
cpp/src/arrow/util/cpu_info.cc:
##
@@ -550,95 +368,64 @@ void ArchVerifyCpuRequirements(const CpuInfo* ci) {}
} // namespace
-struct CpuInfo::Impl {
- int64_t hardware_flags = 0;
- int num_cores = 0;
- int64_t original_hardware_flags = 0;
- Vendor vendor = Vendor::Unknown;
- std::string model_name = "Unknown";
- std::array cache_si
Re: [PR] GH-45331: Use xsimd for CPU feature detection [arrow]
Copilot commented on code in PR #49940:
URL: https://github.com/apache/arrow/pull/49940#discussion_r3348407102
##
cpp/src/arrow/util/cpu_info.cc:
##
@@ -550,95 +365,64 @@ void ArchVerifyCpuRequirements(const CpuInfo* ci) {}
} // namespace
-struct CpuInfo::Impl {
- int64_t hardware_flags = 0;
- int num_cores = 0;
- int64_t original_hardware_flags = 0;
- Vendor vendor = Vendor::Unknown;
- std::string model_name = "Unknown";
- std::array cache_sizes{};
-
- Impl() {
-OsRetrieveCacheSize(&cache_sizes);
-OsRetrieveCpuInfo(&hardware_flags, &vendor, &model_name);
-original_hardware_flags = hardware_flags;
-num_cores =
std::max(static_cast(std::thread::hardware_concurrency()), 1);
-
-// parse user simd level
-auto maybe_env_var = GetEnvVar("ARROW_USER_SIMD_LEVEL");
-if (!maybe_env_var.ok()) {
- return;
-}
-std::string s = *std::move(maybe_env_var);
-std::transform(s.begin(), s.end(), s.begin(),
- [](unsigned char c) { return std::toupper(c); });
-if (!ArchParseUserSimdLevel(s, &hardware_flags)) {
- ARROW_LOG(WARNING) << "Invalid value for ARROW_USER_SIMD_LEVEL: " << s;
-}
- }
+CpuInfo::CpuInfo() {
+ OsRetrieveCacheSize(&cache_sizes_);
+ OsRetrieveCpuInfo(&hardware_flags_, &vendor_);
+ original_hardware_flags_ = hardware_flags_;
+ num_cores_ = std::max(static_cast(std::thread::hardware_concurrency()),
1);
- void EnableFeature(int64_t flag, bool enable) {
-if (!enable) {
- hardware_flags &= ~flag;
-} else {
- // Can't turn something on that can't be supported
- DCHECK_EQ((~original_hardware_flags) & flag, 0);
- hardware_flags |= (flag & original_hardware_flags);
-}
+ // parse user simd level
+ auto maybe_env_var = GetEnvVar("ARROW_USER_SIMD_LEVEL");
+ if (!maybe_env_var.ok()) {
+return;
}
-};
-
-CpuInfo::~CpuInfo() = default;
-
-CpuInfo::CpuInfo() : impl_(new Impl) {}
+ std::string s = *std::move(maybe_env_var);
+ std::transform(s.begin(), s.end(), s.begin(),
+ [](unsigned char c) { return std::toupper(c); });
+ if (!ArchParseUserSimdLevel(s, &hardware_flags_)) {
+ARROW_LOG(WARNING) << "Invalid value for ARROW_USER_SIMD_LEVEL: " << s;
+ }
+}
const CpuInfo* CpuInfo::GetInstance() {
- static CpuInfo cpu_info;
+ static const CpuInfo cpu_info;
return &cpu_info;
}
Review Comment:
Making the singleton instance `static const CpuInfo` turns the underlying
object into a const object. Any existing code that (even indirectly) mutates
`CpuInfo` (e.g., via `EnableFeature` using a `const_cast` on the `const
CpuInfo*` returned by `GetInstance()`) becomes undefined behavior. Keep the
stored singleton non-const (as before) to preserve safe mutability of the
instance (even if callers only see a `const CpuInfo*`).
##
cpp/src/arrow/util/cpu_info.cc:
##
@@ -349,104 +246,22 @@ int64_t LinuxGetCacheSize(int level) {
return static_cast(size);
}
-// Helper function to parse for hardware flags from /proc/cpuinfo
-// values contains a list of space-separated flags. check to see if the flags
we
-// care about are present.
-// Returns a bitmap of flags.
-int64_t LinuxParseCpuFlags(const std::string& values) {
-# if defined(CPUINFO_ARCH_X86) || defined(CPUINFO_ARCH_ARM)
- const struct {
-std::string name;
-int64_t flag;
- } flag_mappings[] = {
-#if defined(CPUINFO_ARCH_X86)
- {"ssse3", CpuInfo::SSSE3}, {"sse4_1", CpuInfo::SSE4_1},
- {"sse4_2", CpuInfo::SSE4_2}, {"popcnt", CpuInfo::POPCNT},
- {"avx", CpuInfo::AVX}, {"avx2", CpuInfo::AVX2},
- {"avx512f", CpuInfo::AVX512F}, {"avx512cd", CpuInfo::AVX512CD},
- {"avx512vl", CpuInfo::AVX512VL}, {"avx512dq", CpuInfo::AVX512DQ},
- {"avx512bw", CpuInfo::AVX512BW}, {"bmi1", CpuInfo::BMI1},
- {"bmi2", CpuInfo::BMI2},
-#elif defined(CPUINFO_ARCH_ARM)
- {"asimd", CpuInfo::ASIMD},
-#endif
- };
- const int64_t num_flags = sizeof(flag_mappings) / sizeof(flag_mappings[0]);
-
- int64_t flags = 0;
- for (int i = 0; i < num_flags; ++i) {
-if (values.find(flag_mappings[i].name) != std::string::npos) {
- flags |= flag_mappings[i].flag;
-}
- }
- return flags;
-# else
- return 0;
-# endif
-}
-
-void OsRetrieveCacheSize(std::array* cache_sizes) {
- for (int i = 0; i < kCacheLevels; ++i) {
+template
+void OsRetrieveCacheSize(std::array* cache_sizes) {
+ static_assert(N >= 3);
+ for (int i = 0; i < static_cast(N); ++i) {
const int64_t cache_size = LinuxGetCacheSize(i);
if (cache_size > 0) {
(*cache_sizes)[i] = cache_size;
Review Comment:
On Linux, `LinuxGetCacheSize(level)` relies on fixed-size lookup tables
(L1/L2/L3). `OsRetrieveCacheSize` now iterates up to `N` (derived from
`CpuInfo::CacheLevel::Last + 1`), but there is no longer a compile-time check
here ensuring `N` matches the number of entries supported by
`LinuxGetCacheSize`. Consider reintroducing a compi
Re: [PR] GH-45331: Use xsimd for CPU feature detection [arrow]
github-actions[bot] commented on PR #49940: URL: https://github.com/apache/arrow/pull/49940#issuecomment-4389120066 :warning: GitHub issue #45331 **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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
[PR] GH-45331: Use xsimd for CPU feature detection [arrow]
AntoinePrv opened a new pull request, #49940: URL: https://github.com/apache/arrow/pull/49940 ### Rationale for this change Use xsimd cpu feature instead of maitaining them here. Stacked on GH-49922. ### What changes are included in this PR? - Use xsimd for CPu feature detection, cache sizes are still using the same implementation - We are loosing CPU model name (which was unused) - Simplify CpuInfo (remove Pimpl pattern) ### Are these changes tested? in CI ### 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
