llvm-project [AArch64] Add getHostCPUFeatures to query for enabled features in cpu…

…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

Jul 04 '24 15:07 neildhickey

@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()));
+        }
+      }
+    }
+  }
 }

Jul 04 '24 15:07 llvmbot

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

Jul 04 '24 15:07 github-actions[bot]

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"

Jul 05 '24 07:07 davemgreen

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.

Jul 05 '24 07:07 neildhickey

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?

Jul 05 '24 07:07 DavidSpickett

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

Jul 24 '24 07:07 neildhickey

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.

Jul 24 '24 08:07 davemgreen

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

Oct 28 '24 16:10 ElvinaYakubova

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