[X86][Driver] Do not add -evex512 for -march=native when the target doesn't support AVX512

[phoebewang]

Users want -march=sandybridge -mavx512f -mavx512vl behaves the same as -march=native -mavx512f -mavx512vl on a sandybridge target.

Fixes: #91076

[llvmbot]

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

@llvm/pr-subscribers-clang-driver

Author: Phoebe Wang (phoebewang)

Changes

Users want -march=sandybridge -mavx512f -mavx512vl behaves the same as -march=native -mavx512f -mavx512vl on a sandybridge target.

Fixes: #91076

---

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

1 Files Affected:

• (modified) clang/lib/Driver/ToolChains/Arch/X86.cpp (+6-2)

diff --git a/clang/lib/Driver/ToolChains/Arch/X86.cpp b/clang/lib/Driver/ToolChains/Arch/X86.cpp
index 53e26a9f8e229..c6d97ef4e971a 100644
--- a/clang/lib/Driver/ToolChains/Arch/X86.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/X86.cpp
@@ -132,10 +132,14 @@ void x86::getX86TargetFeatures(const Driver &D, const llvm::Triple &Triple,
   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 (llvm::sys::getHostCPUFeatures(HostFeatures)) {
+        for (auto &F : HostFeatures) {
+          if (!F.second && F.first() == "evex512")
+            continue;
           Features.push_back(
               Args.MakeArgString((F.second ? "+" : "-") + F.first()));
+        }
+      }
     }
   }
 

[topperc]

Could you make getHostCPUFeatures only write the Features["evex512"] entry if Features["avx512f"] is true? Instead of copying Features["avx512f"]?

[phoebewang]

Could you make getHostCPUFeatures only write the Features["evex512"] entry if Features["avx512f"] is true? Instead of copying Features["avx512f"]?

Good idea, done.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff db78ee0cb82669302a5e0f18a15fd53346a73823 d50ae3b317839f5603b091aa029414b3c3c42259 -- llvm/lib/TargetParser/Host.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/TargetParser/Host.cpp b/llvm/lib/TargetParser/Host.cpp
index c5156c6cb8..ca053c8872 100644
--- a/llvm/lib/TargetParser/Host.cpp
+++ b/llvm/lib/TargetParser/Host.cpp
@@ -1803,7 +1803,7 @@ bool sys::getHostCPUFeatures(StringMap<bool> &Features) {
   // AVX512 is only supported if the OS supports the context save for it.
   Features["avx512f"]    = HasLeaf7 && ((EBX >> 16) & 1) && HasAVX512Save;
   if (Features["avx512f"])
-    Features["evex512"]  = true;
+    Features["evex512"] = true;
   Features["avx512dq"]   = HasLeaf7 && ((EBX >> 17) & 1) && HasAVX512Save;
   Features["rdseed"]     = HasLeaf7 && ((EBX >> 18) & 1);
   Features["adx"]        = HasLeaf7 && ((EBX >> 19) & 1);

[topperc]

LGTM

[phoebewang]

/cherry-pick 87f3407

[llvmbot]

/pull-request #91705

[FireBurn]

@tstellar Can a note be added somewhere about this? Folks upgrading to llvm-18.1.6 will get errors unless they drop -march=native if they were on 18.1.5

[phoebewang]

@tstellar Can a note be added somewhere about this? Folks upgrading to llvm-18.1.6 will get errors unless they drop -march=native if they were on 18.1.5

Although I agree we should add a note, I don't think this patch affects much. The problem only coccurs when combining -march=native with AVX512 options on a non AVX512 target. This actually is not a valid scenario. The compiled binary will contain AVX512 instruction which crashes it on the target. It violates the intention of using -march=native. I also don't see where the error from with 18.1.6. If there were errors, it should come from 18.1.5.

[phoebewang]

@FireBurn Sorry, I just noticed #91719. The above comments were based on the issue #91076.

I didn't look at the source, but the command line doesn't have an AVX512 option. So I'm assuming the code may related function multi-versioning with manually specified target attributes regarding to AVX512 features.

The reason is the same as #91719, but the scenarios may be more common in practice. The defect in previous patch does expose a known issue with the design of EVEX512, which we have noted in https://clang.llvm.org/docs/UsersManual.html#x86

In a word, when user uses target attributes with AVX512 features, they should add an explicit evex512 or no-evex512 for robustness since LLVM 18. Missing it may result in some unexpected codegen or error in some corner case.

[FireBurn]

You'll be probably building 18.1.6 with 18.1.5... and that's when you'll notice the issue. I'm just about finished building 18.1.5 with your patch. So hopefully all those issues will be gone. I'll report back

[phoebewang]

You'll be probably building 18.1.6 with 18.1.5... and that's when you'll notice the issue. I'm just about finished building 18.1.5 with your patch. So hopefully all those issues will be gone. I'll report back

Okay... This is a combined problem which I never thought before. Sorry for the inconvenience!

[FireBurn]

No worries, it's fixed now, llvm, nodejs and libreoffice are all compiling fine again

[tstellar]

@phoebewang Can you add a release note for this on the PR for the release branch and then add the release:note label.

[phoebewang]

@phoebewang Can you add a release note for this on the PR for the release branch and then add the release:note label.

Done.