[llvm][TargetParser] Return StringMap from getHostCPUFeatures

[DavidSpickett]

Previously this took a reference to a map and returned a bool to say whether it succeeded. We can return a StringMap instead, as all callers but 1 simply iterated the map if the bool was true, and passed in empty maps as the starting point.

lldb's lit-cpuid did specifically check whether the call failed, but due to the way the x86 routines work this works out the same as checking if the returned map is empty.

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-backend-arm
@llvm/pr-subscribers-lldb
@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-clang-driver

Author: David Spickett (DavidSpickett)

Changes

Previously this took a reference to a map and returned a bool to say whether it succeeded. This is an optional but with more steps.

The only reason to keep it that way was if someone was appending to an existing map, but all callers made a new map before calling it.

---

Full diff: https://github.com/llvm/llvm-project/pull/97824.diff

8 Files Affected:

• (modified) clang/lib/Driver/ToolChains/Arch/ARM.cpp (+3-3)
• (modified) clang/lib/Driver/ToolChains/Arch/X86.cpp (+3-3)
• (modified) lldb/utils/lit-cpuid/lit-cpuid.cpp (+11-10)
• (modified) llvm/include/llvm/TargetParser/Host.h (+7-7)
• (modified) llvm/lib/CodeGen/CommandFlags.cpp (+6-10)
• (modified) llvm/lib/ExecutionEngine/Orc/JITTargetMachineBuilder.cpp (+4-4)
• (modified) llvm/lib/Target/TargetMachineC.cpp (+2-3)
• (modified) llvm/lib/TargetParser/Host.cpp (+18-11)

diff --git a/clang/lib/Driver/ToolChains/Arch/ARM.cpp b/clang/lib/Driver/ToolChains/Arch/ARM.cpp
index 8ae22cc37a136..77adbf3865ab1 100644
--- a/clang/lib/Driver/ToolChains/Arch/ARM.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -591,9 +591,9 @@ llvm::ARM::FPUKind arm::getARMTargetFeatures(const Driver &D,
 
   // Add CPU features for generic CPUs
   if (CPUName == "native") {
-    llvm::StringMap<bool> HostFeatures;
-    if (llvm::sys::getHostCPUFeatures(HostFeatures))
-      for (auto &F : HostFeatures)
+    if (std::optional<llvm::StringMap<bool>> HostFeatures =
+            llvm::sys::getHostCPUFeatures())
+      for (auto &F : *HostFeatures)
         Features.push_back(
             Args.MakeArgString((F.second ? "+" : "-") + F.first()));
   } else if (!CPUName.empty()) {
diff --git a/clang/lib/Driver/ToolChains/Arch/X86.cpp b/clang/lib/Driver/ToolChains/Arch/X86.cpp
index 92821b2a82dae..e4adfcac23ca0 100644
--- a/clang/lib/Driver/ToolChains/Arch/X86.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/X86.cpp
@@ -131,9 +131,9 @@ void x86::getX86TargetFeatures(const Driver &D, const llvm::Triple &Triple,
   // If -march=native, autodetect the feature list.
   if (const Arg *A = Args.getLastArg(clang::driver::options::OPT_march_EQ)) {
     if (StringRef(A->getValue()) == "native") {
-      llvm::StringMap<bool> HostFeatures;
-      if (llvm::sys::getHostCPUFeatures(HostFeatures))
-        for (auto &F : HostFeatures)
+      if (std::optional<llvm::StringMap<bool>> HostFeatures =
+              llvm::sys::getHostCPUFeatures())
+        for (auto &F : *HostFeatures)
           Features.push_back(
               Args.MakeArgString((F.second ? "+" : "-") + F.first()));
     }
diff --git a/lldb/utils/lit-cpuid/lit-cpuid.cpp b/lldb/utils/lit-cpuid/lit-cpuid.cpp
index be322cb6aa42a..16743164e6a5d 100644
--- a/lldb/utils/lit-cpuid/lit-cpuid.cpp
+++ b/lldb/utils/lit-cpuid/lit-cpuid.cpp
@@ -15,22 +15,23 @@
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TargetParser/Host.h"
 
+#include <optional>
+
 using namespace llvm;
 
 int main(int argc, char **argv) {
 #if defined(__i386__) || defined(_M_IX86) || \
     defined(__x86_64__) || defined(_M_X64)
-  StringMap<bool> features;
-
-  if (!sys::getHostCPUFeatures(features))
+  if (std::optional<StringMap<bool>> features =
+          sys::getHostCPUFeatures(features)) {
+    if ((*features)["sse"])
+      outs() << "sse\n";
+    if ((*features)["avx"])
+      outs() << "avx\n";
+    if ((*features)["avx512f"])
+      outs() << "avx512f\n";
+  } else
     return 1;
-
-  if (features["sse"])
-    outs() << "sse\n";
-  if (features["avx"])
-    outs() << "avx\n";
-  if (features["avx512f"])
-    outs() << "avx512f\n";
 #endif
 
   return 0;
diff --git a/llvm/include/llvm/TargetParser/Host.h b/llvm/include/llvm/TargetParser/Host.h
index af72045a8fe67..d68655835a323 100644
--- a/llvm/include/llvm/TargetParser/Host.h
+++ b/llvm/include/llvm/TargetParser/Host.h
@@ -13,6 +13,7 @@
 #ifndef LLVM_TARGETPARSER_HOST_H
 #define LLVM_TARGETPARSER_HOST_H
 
+#include <optional>
 #include <string>
 
 namespace llvm {
@@ -47,13 +48,12 @@ namespace sys {
   /// The particular format of the names are target dependent, and suitable for
   /// passing as -mattr to the target which matches the host.
   ///
-  /// \param Features - A string mapping feature names to either
-  /// true (if enabled) or false (if disabled). This routine makes no guarantees
-  /// about exactly which features may appear in this map, except that they are
-  /// all valid LLVM feature names.
-  ///
-  /// \return - True on success.
-  bool getHostCPUFeatures(StringMap<bool, MallocAllocator> &Features);
+  /// \return - If feature detection succeeds, a string map mapping feature
+  /// names to either true (if enabled) or false (if disabled). This routine
+  /// makes no guarantees about exactly which features may appear in this map,
+  /// except that they are all valid LLVM feature names. If feature detection
+  /// fails, an empty optional is returned.
+  std::optional<StringMap<bool, MallocAllocator>> getHostCPUFeatures();
 
   /// This is a function compatible with cl::AddExtraVersionPrinter, which adds
   /// info about the current target triple and detected CPU.
diff --git a/llvm/lib/CodeGen/CommandFlags.cpp b/llvm/lib/CodeGen/CommandFlags.cpp
index 8fc65d78ff2c9..b881f1289e9f3 100644
--- a/llvm/lib/CodeGen/CommandFlags.cpp
+++ b/llvm/lib/CodeGen/CommandFlags.cpp
@@ -624,12 +624,10 @@ std::string codegen::getFeaturesStr() {
   // This is necessary for x86 where the CPU might not support all the
   // features the autodetected CPU name lists in the target. For example,
   // not all Sandybridge processors support AVX.
-  if (getMCPU() == "native") {
-    StringMap<bool> HostFeatures;
-    if (sys::getHostCPUFeatures(HostFeatures))
-      for (const auto &[Feature, IsEnabled] : HostFeatures)
+  if (getMCPU() == "native")
+    if (std::optional<StringMap<bool>> HostFeatures = sys::getHostCPUFeatures())
+      for (const auto &[Feature, IsEnabled] : *HostFeatures)
         Features.AddFeature(Feature, IsEnabled);
-  }
 
   for (auto const &MAttr : getMAttrs())
     Features.AddFeature(MAttr);
@@ -644,12 +642,10 @@ std::vector<std::string> codegen::getFeatureList() {
   // This is necessary for x86 where the CPU might not support all the
   // features the autodetected CPU name lists in the target. For example,
   // not all Sandybridge processors support AVX.
-  if (getMCPU() == "native") {
-    StringMap<bool> HostFeatures;
-    if (sys::getHostCPUFeatures(HostFeatures))
-      for (const auto &[Feature, IsEnabled] : HostFeatures)
+  if (getMCPU() == "native")
+    if (std::optional<StringMap<bool>> HostFeatures = sys::getHostCPUFeatures())
+      for (const auto &[Feature, IsEnabled] : *HostFeatures)
         Features.AddFeature(Feature, IsEnabled);
-  }
 
   for (auto const &MAttr : getMAttrs())
     Features.AddFeature(MAttr);
diff --git a/llvm/lib/ExecutionEngine/Orc/JITTargetMachineBuilder.cpp b/llvm/lib/ExecutionEngine/Orc/JITTargetMachineBuilder.cpp
index 540728ea0fd99..5f5a8f7168998 100644
--- a/llvm/lib/ExecutionEngine/Orc/JITTargetMachineBuilder.cpp
+++ b/llvm/lib/ExecutionEngine/Orc/JITTargetMachineBuilder.cpp
@@ -28,10 +28,10 @@ Expected<JITTargetMachineBuilder> JITTargetMachineBuilder::detectHost() {
   // Retrieve host CPU name and sub-target features and add them to builder.
   // Relocation model, code model and codegen opt level are kept to default
   // values.
-  llvm::StringMap<bool> FeatureMap;
-  llvm::sys::getHostCPUFeatures(FeatureMap);
-  for (auto &Feature : FeatureMap)
-    TMBuilder.getFeatures().AddFeature(Feature.first(), Feature.second);
+  if (std::optional<StringMap<bool>> FeatureMap =
+          llvm::sys::getHostCPUFeatures())
+    for (auto &Feature : *FeatureMap)
+      TMBuilder.getFeatures().AddFeature(Feature.first(), Feature.second);
 
   TMBuilder.setCPU(std::string(llvm::sys::getHostCPUName()));
 
diff --git a/llvm/lib/Target/TargetMachineC.cpp b/llvm/lib/Target/TargetMachineC.cpp
index 80024f9a6d5df..340ae50285951 100644
--- a/llvm/lib/Target/TargetMachineC.cpp
+++ b/llvm/lib/Target/TargetMachineC.cpp
@@ -363,10 +363,9 @@ char *LLVMGetHostCPUName(void) {
 
 char *LLVMGetHostCPUFeatures(void) {
   SubtargetFeatures Features;
-  StringMap<bool> HostFeatures;
 
-  if (sys::getHostCPUFeatures(HostFeatures))
-    for (const auto &[Feature, IsEnabled] : HostFeatures)
+  if (std::optional<StringMap<bool>> HostFeatures = sys::getHostCPUFeatures())
+    for (const auto &[Feature, IsEnabled] : *HostFeatures)
       Features.AddFeature(Feature, IsEnabled);
 
   return strdup(Features.getString().c_str());
diff --git a/llvm/lib/TargetParser/Host.cpp b/llvm/lib/TargetParser/Host.cpp
index 2ea56746aff24..4cf06505e902e 100644
--- a/llvm/lib/TargetParser/Host.cpp
+++ b/llvm/lib/TargetParser/Host.cpp
@@ -1710,15 +1710,17 @@ VendorSignatures getVendorSignature(unsigned *MaxLeaf) {
 
 #if defined(__i386__) || defined(_M_IX86) || \
     defined(__x86_64__) || defined(_M_X64)
-bool sys::getHostCPUFeatures(StringMap<bool> &Features) {
+std::optional<StringMap<bool>> sys::getHostCPUFeatures() {
   unsigned EAX = 0, EBX = 0, ECX = 0, EDX = 0;
   unsigned MaxLevel;
 
   if (getX86CpuIDAndInfo(0, &MaxLevel, &EBX, &ECX, &EDX) || MaxLevel < 1)
-    return false;
+    return {};
 
   getX86CpuIDAndInfo(1, &EAX, &EBX, &ECX, &EDX);
 
+  StringMap<bool> Features;
+
   Features["cx8"]    = (EDX >>  8) & 1;
   Features["cmov"]   = (EDX >> 15) & 1;
   Features["mmx"]    = (EDX >> 23) & 1;
@@ -1903,13 +1905,13 @@ bool sys::getHostCPUFeatures(StringMap<bool> &Features) {
   Features["avx10.1-512"] =
       Features["avx10.1-256"] && HasLeaf24 && ((EBX >> 18) & 1);
 
-  return true;
+  return Features;
 }
 #elif defined(__linux__) && (defined(__arm__) || defined(__aarch64__))
-bool sys::getHostCPUFeatures(StringMap<bool> &Features) {
+std::optional<StringMap<bool>> sys::getHostCPUFeatures() {
   std::unique_ptr<llvm::MemoryBuffer> P = getProcCpuinfoContent();
   if (!P)
-    return false;
+    return {};
 
   SmallVector<StringRef, 32> Lines;
   P->getBuffer().split(Lines, "\n");
@@ -1929,6 +1931,7 @@ bool sys::getHostCPUFeatures(StringMap<bool> &Features) {
   uint32_t crypto = 0;
 #endif
 
+  StringMap<bool> Features;
   for (unsigned I = 0, E = CPUFeatures.size(); I != E; ++I) {
     StringRef LLVMFeatureStr = StringSwitch<StringRef>(CPUFeatures[I])
 #if defined(__aarch64__)
@@ -1972,10 +1975,12 @@ bool sys::getHostCPUFeatures(StringMap<bool> &Features) {
     Features["crypto"] = true;
 #endif
 
-  return true;
+  return Features;
 }
 #elif defined(_WIN32) && (defined(__aarch64__) || defined(_M_ARM64))
-bool sys::getHostCPUFeatures(StringMap<bool> &Features) {
+std::optional<StringMap<bool>> sys::getHostCPUFeatures() {
+  std::optional<StringMap<bool>> Features;
+
   if (IsProcessorFeaturePresent(PF_ARM_NEON_INSTRUCTIONS_AVAILABLE))
     Features["neon"] = true;
   if (IsProcessorFeaturePresent(PF_ARM_V8_CRC32_INSTRUCTIONS_AVAILABLE))
@@ -1983,16 +1988,18 @@ bool sys::getHostCPUFeatures(StringMap<bool> &Features) {
   if (IsProcessorFeaturePresent(PF_ARM_V8_CRYPTO_INSTRUCTIONS_AVAILABLE))
     Features["crypto"] = true;
 
-  return true;
+  return Features;
 }
 #elif defined(__linux__) && defined(__loongarch__)
 #include <sys/auxv.h>
-bool sys::getHostCPUFeatures(StringMap<bool> &Features) {
+std::optional<StringMap<bool>> sys::getHostCPUFeatures() {
   unsigned long hwcap = getauxval(AT_HWCAP);
   bool HasFPU = hwcap & (1UL << 3); // HWCAP_LOONGARCH_FPU
   uint32_t cpucfg2 = 0x2;
   __asm__("cpucfg %[cpucfg2], %[cpucfg2]\n\t" : [cpucfg2] "+r"(cpucfg2));
 
+  std::optional<StringMap<bool>> Features;
+
   Features["f"] = HasFPU && (cpucfg2 & (1U << 1)); // CPUCFG.2.FP_SP
   Features["d"] = HasFPU && (cpucfg2 & (1U << 2)); // CPUCFG.2.FP_DP
 
@@ -2000,10 +2007,10 @@ bool sys::getHostCPUFeatures(StringMap<bool> &Features) {
   Features["lasx"] = hwcap & (1UL << 5); // HWCAP_LOONGARCH_LASX
   Features["lvz"] = hwcap & (1UL << 9);  // HWCAP_LOONGARCH_LVZ
 
-  return true;
+  return Features;
 }
 #else
-bool sys::getHostCPUFeatures(StringMap<bool> &Features) { return false; }
+std::optional<StringMap<bool>> sys::getHostCPUFeatures() { return {}; }
 #endif
 
 #if __APPLE__

[bulbazord]

From an API standpoint, there doesn't actually seem to be a huge difference between an empty StringMap and an optional with an empty string map in it right? Why not return a map every time?

[DavidSpickett]

Fair point, iirc all the callers go on to iterate the map so no one is handling failure in a special way. I'll update this.

[DavidSpickett]

Why not return a map every time?

Done.

[phoebewang]

LGTM.

[bulbazord]

LGTM, thanks!

[satmandu]

Worth noting that this affects mesa too: https://gitlab.freedesktop.org/mesa/mesa/-/issues/11896