Copilot commented on code in PR #49940:
URL: https://github.com/apache/arrow/pull/49940#discussion_r3363388457
##########
cpp/src/arrow/util/cpu_info.cc:
##########
@@ -349,104 +249,29 @@ int64_t LinuxGetCacheSize(int level) {
return static_cast<int64_t>(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<int64_t, kCacheLevels>* cache_sizes) {
- for (int i = 0; i < kCacheLevels; ++i) {
+template <std::size_t N>
+void OsRetrieveCacheSize(std::array<int64_t, N>* cache_sizes) {
+ static_assert(N <= 3);
+ for (int i = 0; i < static_cast<int>(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 <sys/sysctl.h>
-#endif
-
-#ifndef _MSC_VER
-# include <unistd.h>
-#endif
-
-#ifdef _WIN32
-# include <intrin.h>
-
-# include "arrow/util/windows_compatibility.h"
-#endif
-
#include <algorithm>
#include <array>
-#include <bitset>
#include <cctype>
#include <cerrno>
#include <cstdint>
-#include <fstream>
-#include <memory>
#include <optional>
#include <string>
#include <thread>
+#include <xsimd/xsimd.hpp>
+
#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 <fstream>
#endif
-namespace arrow {
-namespace internal {
+#ifdef __APPLE__
+# include <sys/sysctl.h>
+#endif
+
+#ifndef _MSC_VER
+# include <unistd.h>
+#endif
+
+#ifdef _WIN32
+# include <intrin.h>
+
+# include "arrow/util/windows_compatibility.h"
+#endif
+
+namespace arrow::internal {
namespace {
-constexpr int kCacheLevels = static_cast<int>(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<xsimd::x86_cpuid_leaf7::ebx>(31);
+ *hardware_flags |= cpu_x86.leaf7().all_bits_set<avx512vl>() ?
CpuInfo::AVX512VL : 0;
+ }
Review Comment:
This function unconditionally references x86-only xsimd APIs
(`xsimd::x86_cpu_features_backend_default`, `xsimd::x86_cpuid_leaf7`) inside
`OsRetrieveCpuInfo`. Even though it’s runtime-gated, these names still must
compile on non-x86 builds (e.g., ARM64), and likely won’t. Guard the x86-only
block with a compile-time check (e.g., `#if XSIMD_TARGET_X86`) and keep non-x86
builds free of x86-only types.
--
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]
