[X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization

[craig.topper]

Summary

This patch defaults to -mtune=generic unless -march is present. If -march is present we'll use the empty string unless its overridden by mtune. The back should use the target cpu if the tune-cpu isn't present.

It also adds AST serialization support to fix some tests that emit AST and parse it back. These tests diff the IR against the output from not going through AST. So if we don't serialize the tune CPU we fail the diff.

[erichkeane]

clang/lib/Serialization/ASTReader.cpp

395

What does this string do? Does there have to be a test for it?

[craig.topper]

clang/lib/Serialization/ASTReader.cpp

395

It seems to be used by a diagnostic. But I couldn't find any tests for the diagnostic for "target CPU" that I could copy from.

[nikic]

This change has a 0.3% compile-time regression (https://llvm-compile-time-tracker.com/compare.php?from=0c55889d809027136048a0d144209a2bc282e7fc&to=71f3169e1baeff262583b35ef88f8fb6df7be85e&stat=instructions) and a 0.3% max-rss regression (https://llvm-compile-time-tracker.com/compare.php?from=0c55889d809027136048a0d144209a2bc282e7fc&to=71f3169e1baeff262583b35ef88f8fb6df7be85e&stat=max-rss).

Is that expected? The diff looks pretty harmless to me.

[craig.topper]

In D86488#2241176, @nikic wrote:

This change has a 0.3% compile-time regression (https://llvm-compile-time-tracker.com/compare.php?from=0c55889d809027136048a0d144209a2bc282e7fc&to=71f3169e1baeff262583b35ef88f8fb6df7be85e&stat=instructions) and a 0.3% max-rss regression (https://llvm-compile-time-tracker.com/compare.php?from=0c55889d809027136048a0d144209a2bc282e7fc&to=71f3169e1baeff262583b35ef88f8fb6df7be85e&stat=max-rss).

Is that expected? The diff looks pretty harmless to me.

No it wasnt expected. There was already a comment on the commit about it. The settings it’s selecting should match the default we get with no -march on Linux. The only thing I’ve found so far is extra time to read the function attribute from getSubtargetImpl which happen every time getTTI is called. And extra time to add the function attribute in clang. The strange thing is it seems to have regressed O0 even so that rules out a lot of optimization related code. Any information you can provide would be helpful.

clang/lib/Basic/Targets/X86.h

public:
   }
 
   bool isValidCPUName(StringRef Name) const override {
     bool Only64Bit = getTriple().getArch() != llvm::Triple::x86;
     return llvm::X86::parseArchX86(Name, Only64Bit) != llvm::X86::CK_None;
   }
 
   bool isValidTuneCPUName(StringRef Name) const override {
+    if (Name == "generic")
+      return true;
+
     // Allow 32-bit only CPUs regardless of 64-bit mode unlike isValidCPUName.
     // NOTE: gcc rejects 32-bit mtune CPUs in 64-bit mode. But being lenient
     // since mtune was ignored by clang for so long.
     return llvm::X86::parseArchX86(Name) != llvm::X86::CK_None;
   }
 
   void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const override;
   void fillValidTuneCPUList(SmallVectorImpl<StringRef> &Values) const override;

clang/lib/Driver/ToolChains/Clang.cpp

void Clang::AddX86TargetArgs(const ArgList &Args,
   // Set flags to support MCU ABI.
   if (Args.hasFlag(options::OPT_miamcu, options::OPT_mno_iamcu, false)) {
     CmdArgs.push_back("-mfloat-abi");
     CmdArgs.push_back("soft");
     CmdArgs.push_back("-mstack-alignment=4");
   }
 
   // Handle -mtune.
-  // FIXME: We should default to "generic" unless -march is set to match gcc.
+
+  // Default to "generic" unless -march is present.
+  std::string TuneCPU;
+  if (!Args.hasArg(clang::driver::options::OPT_march_EQ))
+    TuneCPU = "generic";
+
+  // Override based on -mtune.
   if (const Arg *A = Args.getLastArg(clang::driver::options::OPT_mtune_EQ)) {
     StringRef Name = A->getValue();
 
-    if (Name == "native")
+    if (Name == "native") {
       Name = llvm::sys::getHostCPUName();
+      if (!Name.empty())
+        TuneCPU = std::string(Name);
+    } else
+      TuneCPU = std::string(Name);
+  }
 
-    // Ignore generic either from getHostCPUName or from command line.
-    // FIXME: We need to support this eventually but isValidCPUName and the
-    // backend aren't ready for it yet.
-    if (Name != "generic") {
-      CmdArgs.push_back("-tune-cpu");
-      CmdArgs.push_back(Args.MakeArgString(Name));
-    }
+  if (!TuneCPU.empty()) {
+    CmdArgs.push_back(Args.MakeArgString(TuneCPU));
   }
 }
 
 void Clang::AddHexagonTargetArgs(const ArgList &Args,
                                  ArgStringList &CmdArgs) const {
   CmdArgs.push_back("-mqdsp6-compat");
   CmdArgs.push_back("-Wreturn-type");
 

clang/lib/Frontend/CompilerInvocation.cpp

for (StringRef Feature : LangOpts->ModuleFeatures)
     code = hash_combine(code, Feature);
 
   code = hash_combine(code, LangOpts->ObjCRuntime);
   const auto &BCN = LangOpts->CommentOpts.BlockCommandNames;
   code = hash_combine(code, hash_combine_range(BCN.begin(), BCN.end()));
 
   // Extend the signature with the target options.
   code = hash_combine(code, TargetOpts->Triple, TargetOpts->CPU,
-                      TargetOpts->ABI);
+                      TargetOpts->TuneCPU, TargetOpts->ABI);
   for (const auto &FeatureAsWritten : TargetOpts->FeaturesAsWritten)
     code = hash_combine(code, FeatureAsWritten);
 
   // Extend the signature with preprocessor options.
   const PreprocessorOptions &ppOpts = getPreprocessorOpts();
   const HeaderSearchOptions &hsOpts = getHeaderSearchOpts();
   code = hash_combine(code, ppOpts.UsePredefines, ppOpts.DetailedRecord);
 

clang/lib/Frontend/FrontendActions.cpp

#include "clang/Basic/LangOptions.def"
       return false;
     }
 
     bool ReadTargetOptions(const TargetOptions &TargetOpts, bool Complain,
                            bool AllowCompatibleDifferences) override {
       Out.indent(2) << "Target options:\n";
       Out.indent(4) << "  Triple: " << TargetOpts.Triple << "\n";
       Out.indent(4) << "  CPU: " << TargetOpts.CPU << "\n";
+      Out.indent(4) << "  TuneCPU: " << TargetOpts.TuneCPU << "\n";
       Out.indent(4) << "  ABI: " << TargetOpts.ABI << "\n";
 
       if (!TargetOpts.FeaturesAsWritten.empty()) {
         Out.indent(4) << "Target features:\n";
         for (unsigned I = 0, N = TargetOpts.FeaturesAsWritten.size();
              I != N; ++I) {
           Out.indent(6) << TargetOpts.FeaturesAsWritten[I] << "\n";
         }

clang/lib/Serialization/ASTReader.cpp

#define CHECK_TARGET_OPT(Field, Name)                             \
 
   // The triple and ABI must match exactly.
   CHECK_TARGET_OPT(Triple, "target");
   CHECK_TARGET_OPT(ABI, "target ABI");
 
   // We can tolerate different CPUs in many cases, notably when one CPU
   // supports a strict superset of another. When allowing compatible
   // differences skip this check.
-  if (!AllowCompatibleDifferences)
+  if (!AllowCompatibleDifferences) {
     CHECK_TARGET_OPT(CPU, "target CPU");
+    CHECK_TARGET_OPT(TuneCPU, "tune CPU");
+  }
 
 #undef CHECK_TARGET_OPT
 
   // Compare feature sets.
   SmallVector<StringRef, 4> ExistingFeatures(
                                              ExistingTargetOpts.FeaturesAsWritten.begin(),
                                              ExistingTargetOpts.FeaturesAsWritten.end());
   SmallVector<StringRef, 4> ReadFeatures(TargetOpts.FeaturesAsWritten.begin(),

 
 bool ASTReader::ParseTargetOptions(const RecordData &Record, bool Complain,
                                    ASTReaderListener &Listener,
                                    bool AllowCompatibleDifferences) {
   unsigned Idx = 0;
   TargetOptions TargetOpts;
   TargetOpts.Triple = ReadString(Record, Idx);
   TargetOpts.CPU = ReadString(Record, Idx);
+  TargetOpts.TuneCPU = ReadString(Record, Idx);
   TargetOpts.ABI = ReadString(Record, Idx);
   for (unsigned N = Record[Idx++]; N; --N) {
     TargetOpts.FeaturesAsWritten.push_back(ReadString(Record, Idx));
   }
   for (unsigned N = Record[Idx++]; N; --N) {
     TargetOpts.Features.push_back(ReadString(Record, Idx));
   }
 

clang/lib/Serialization/ASTWriter.cpp

#include "clang/Basic/Sanitizers.def"
   Stream.EmitRecord(LANGUAGE_OPTIONS, Record);
 
   // Target options.
   Record.clear();
   const TargetInfo &Target = Context.getTargetInfo();
   const TargetOptions &TargetOpts = Target.getTargetOpts();
   AddString(TargetOpts.Triple, Record);
   AddString(TargetOpts.CPU, Record);
+  AddString(TargetOpts.TuneCPU, Record);
   AddString(TargetOpts.ABI, Record);
   Record.push_back(TargetOpts.FeaturesAsWritten.size());
   for (unsigned I = 0, N = TargetOpts.FeaturesAsWritten.size(); I != N; ++I) {
     AddString(TargetOpts.FeaturesAsWritten[I], Record);
   }
   Record.push_back(TargetOpts.Features.size());
   for (unsigned I = 0, N = TargetOpts.Features.size(); I != N; ++I) {
     AddString(TargetOpts.Features[I], Record);

clang/test/Driver/x86-mtune.c

 // Ensure we support the -mtune flag.
-//
+
+// Default mtune should be generic.
+// RUN: %clang -target x86_64-unknown-unknown -c -### %s 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=notune
+// notune: "-tune-cpu" "generic"
+
+// RUN: %clang -target x86_64-unknown-unknown -c -### %s -mtune=generic 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=generic
+// generic: "-tune-cpu" "generic"
+
 // RUN: %clang -target x86_64-unknown-unknown -c -### %s -mtune=nocona 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=nocona
 // nocona: "-tune-cpu" "nocona"
 
 // Unlike march we allow 32-bit only cpus with mtune.
 
 // RUN: %clang -target x86_64-unknown-unknown -c -### %s -mtune=i686 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=i686
 // i686: "-tune-cpu" "i686"
 
 // RUN: %clang -target x86_64-unknown-unknown -c -### %s -mtune=pentium4 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=pentium4
 // pentium4: "-tune-cpu" "pentium4"
 
 // RUN: %clang -target x86_64-unknown-unknown -c -### %s -mtune=athlon 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=athlon
 // athlon: "-tune-cpu" "athlon"
 
+// Check interaction between march and mtune.
+
+// -march should remove default mtune generic.
+// RUN: %clang -target x86_64-unknown-unknown -c -### %s -march=core2 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=marchcore2
+// marchcore2: "-target-cpu" "core2"
+// marchcore2-NOT: "-tune-cpu"
+
+// -march should remove default mtune generic.
+// RUN: %clang -target x86_64-unknown-unknown -c -### %s -march=core2 -mtune=nehalem 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=marchmtune
+// marchmtune: "-target-cpu" "core2"
+// mmarchmtune: "-tune-cpu" "nehalem"

clang/test/Modules/module_file_info.m

 // CHECK:   Objective-C: Yes
 // CHECK:   modules semantics: Yes
 // CHECK:   Module features:
 // CHECK:     myfeature
 
 // CHECK: Target options:
 // CHECK:     Triple:
 // CHECK:     CPU:
+// CHECK:     TuneCPU:
 // CHECK:     ABI:
 
 // CHECK: Header search options:
 // CHECK:   System root [-isysroot=]: '/'
 // CHECK:   Resource dir [ -resource-dir=]: '{{.*}}clang{{.*}}'
 // CHECK:   Use builtin include directories [-nobuiltininc]: Yes
 // CHECK:   Use standard system include directories [-nostdinc]: No
 // CHECK:   Use standard C++ include directories [-nostdinc++]: Yes