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<int64_t, kCacheLevels> cache_sizes{};
-
- Impl() {
- OsRetrieveCacheSize(&cache_sizes);
- OsRetrieveCpuInfo(&hardware_flags, &vendor, &model_name);
- original_hardware_flags = hardware_flags;
- num_cores =
std::max(static_cast<int>(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<int>(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<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);
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 compile-time invariant (e.g.,
`static_assert(kCacheLevels == 3)` for the Linux path) or bounding the loop to
the supported number of levels so the code can’t silently become out-of-bounds
if cache levels are extended.
##########
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 is compiled on all targets, but the block uses x86-specific
xsimd types/functions (`x86_cpu_features_backend_default`, `x86_cpuid_leaf7`).
Even though it’s guarded by a runtime `if (avx512f)`, it can still fail to
compile on non-x86 builds. Wrap the x86-only code in a compile-time guard
(e.g., `#if XSIMD_TARGET_X86`) and keep a safe fallback when building for other
architectures.
##########
cpp/src/arrow/util/cpu_info.cc:
##########
@@ -110,7 +153,7 @@ void OsRetrieveCacheSize(std::array<int64_t, kCacheLevels>*
cache_sizes) {
while (offset + sizeof(SYSTEM_LOGICAL_PROCESSOR_INFORMATION) <= buffer_size)
{
if (RelationCache == buffer_position->Relationship) {
PCACHE_DESCRIPTOR cache = &buffer_position->Cache;
- if (cache->Level >= 1 && cache->Level <= kCacheLevels) {
+ if (cache->Level >= 1 && cache->Level <= N) {
Review Comment:
`N` is `std::size_t` while `cache->Level` is typically an integer type from
the Windows API; this can trigger signed/unsigned comparison warnings. Consider
casting `N` to the appropriate integral type (or casting `cache->Level` to
`std::size_t`) before comparison to keep builds warning-clean.
##########
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 change both alters the public API (`const std::string&` ->
`std::string_view`) and regresses behavior to always return \"Unknown\" while
the docstring still claims it returns the CPU model name. To avoid breaking
downstream code and to keep the API truthful, either (a) preserve the original
signature/behavior by storing a `std::string model_name_` populated by
OS-specific means, or (b) keep the old API and deprecate it if model name is
being intentionally removed. At minimum, update the documentation to reflect
the new guaranteed behavior if keeping this implementation.
--
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]
