[clang][AArch64] Add getHostCPUFeatures to query for enabled features in cpu info #97749

neildhickey · 2024-07-04T15:45:58Z

This pull request adds code to call getHostCPUInfo into the AArch64 Target Parser to query the cpuinfo for the device in the case where we are compiling with -mcpu=native

llvmbot · 2024-07-04T15:46:28Z

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-clang

Author: None (neildhickey)

Changes

…info

This pull request adds code to call getHostCPUInfo into the AArch64 Target Parser to query the cpuinfo for the device in the case where we are compiling with -mcpu=native

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

1 Files Affected:

(modified) clang/lib/Driver/ToolChains/Arch/AArch64.cpp (+17)

diff --git a/clang/lib/Driver/ToolChains/Arch/AArch64.cpp b/clang/lib/Driver/ToolChains/Arch/AArch64.cpp
index ec248b80251ea3..2862c297622fa9 100644
--- a/clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -445,4 +445,21 @@ void aarch64::getAArch64TargetFeatures(const Driver &D,
 
   if (Args.getLastArg(options::OPT_mno_bti_at_return_twice))
     Features.push_back("+no-bti-at-return-twice");
+
+  // Parse AArch64 CPU Features
+  const Arg *CPUArg = Args.getLastArg(options::OPT_mcpu_EQ);
+  StringRef CPUName;
+
+  if (CPUArg) {
+    CPUName = CPUArg->getValue();
+    if (CPUName == "native") {
+      llvm::StringMap<bool> HostFeatures;
+      if (llvm::sys::getHostCPUFeatures(HostFeatures)) {
+        for (auto &F : HostFeatures) {
+          Features.push_back(
+            Args.MakeArgString((F.second ? "+" : "-") + F.first()));
+        }
+      }
+    }
+  }
 }

github-actions · 2024-07-04T15:49:44Z

✅ With the latest revision this PR passed the C/C++ code formatter.

madhur13490 · 2024-07-05T06:18:08Z

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

+            Args.MakeArgString((F.second ? "+" : "-") + F.first()));
+        }
+      }
+    }


You can use early return here. Logically stating

if (!CPUArg) return; if (CPUName != "native") return; if (!llvm::sys::getHostCPUFeatures(HostFeatures)) return;

and then iterate over HostFeatures. I think it will be cleaner.

davemgreen · 2024-07-05T07:14:25Z

Hi this sounds like a good idea to me. Note that the implementation of getHostCPUFeatures isn't amazing for AArch64 at the moment, there was an attempt to fix it up in #95694 (but thaat has gone a bit quiet). One point we noticed is that it could end up turning "aes+sha2" into "crypto" and "crypto" back into "sha2+aes+sha3+sm4", as it uses the old meaning of "crypto"

neildhickey · 2024-07-05T07:39:44Z

Thanks for the review comments @madhur13490 and @davemgreen I plan to improve getHostCPUFeatures for AArach64 as well, this is just the first phase to add the function call.

DavidSpickett · 2024-07-05T07:59:20Z

Looks ok to me, X86 and ARM already do this.

One point we noticed is that it could end up turning "aes+sha2" into "crypto" and "crypto" back into "sha2+aes+sha3+sm4", as it uses the old meaning of "crypto"

Then this needs to be fixed before this PR can go in, we don't want to be making binaries folks can't run. @davemgreen do we have a way that @neildhickey can reproduce that easily?

tmatheson-arm · 2024-07-05T08:50:18Z

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

+        for (auto &F : HostFeatures) {
+          Features.push_back(
+            Args.MakeArgString((F.second ? "+" : "-") + F.first()));
+        }


Suggested change

for (auto &F : HostFeatures) {

Features.push_back(

Args.MakeArgString((F.second ? "+" : "-") + F.first()));

}

for (auto &[Name, Enabled] : HostFeatures)

Features.push_back(

Args.MakeArgString((Enabled ? "+" : "-") + Name));

tmatheson-arm · 2024-07-05T08:57:35Z

clang/lib/Driver/ToolChains/Arch/AArch64.cpp

@@ -445,4 +445,21 @@ void aarch64::getAArch64TargetFeatures(const Driver &D,

  if (Args.getLastArg(options::OPT_mno_bti_at_return_twice))
    Features.push_back("+no-bti-at-return-twice");
+
+  // Parse AArch64 CPU Features
+  const Arg *CPUArg = Args.getLastArg(options::OPT_mcpu_EQ);


Feels like this should be done in getAArch64ArchFeaturesFromMcpu/DecodeAArch64Mcpu

neildhickey · 2024-07-24T07:58:15Z

Looks ok to me, X86 and ARM already do this.

One point we noticed is that it could end up turning "aes+sha2" into "crypto" and "crypto" back into "sha2+aes+sha3+sm4", as it uses the old meaning of "crypto"

Then this needs to be fixed before this PR can go in, we don't want to be making binaries folks can't run. @davemgreen do we have a way that @neildhickey can reproduce that easily?

Perhaps this is the part of code causing the issue

#if defined(__aarch64__)
  // If we have all crypto bits we can add the feature
  if (crypto == (CAP_AES | CAP_PMULL | CAP_SHA1 | CAP_SHA2))
    Features["crypto"] = true;
#endif

If so I can change this to remove the addition of crypto and just assert the features found in the cpuinfo file

davemgreen · 2024-07-24T08:05:18Z

It is that bit of code, yeah. I don't know of a way to reproduce this without logging into different machines with different sets of options and trying it.

If we had a way to test/mock that various /proc/cpuinfo files gave us the correct results, that would be helpful in giving us more confidence it was working as intended.

davemgreen

Thanks for adding the tests. They look like a useful addition.

llvm/lib/TargetParser/Host.cpp

…info

ElvinaYakubova · 2024-10-28T16:20:36Z

Sorry for the delays in response. I addressed the comments and rebased the branch

davemgreen

Thanks - LGTM

clang/test/Driver/aarch64-mcpu-native.c

Add LLVM_CPUINFO environment variable to test mock /proc/cpuinfo files for -mcpu=native

llvm-ci · 2024-10-29T13:49:37Z

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-gcc-ubuntu running on sie-linux-worker3 while building clang,llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/7513