[X86] Add support 'tune' in target attribute

[craig.topper]

Summary

This adds parsing and codegen support for tune in target attribute.

I've implemented this so that arch in the target attribute implicitly disables tune from the command line. I'm not sure what gcc does here. But since -march implies -mtune. I assume 'arch' in the target attribute implies tune in the target attribute.

[erichkeane]

CFE changes look right to me. Please give the rest of the reviews a little while to take a look though

[aaron.ballman]

LGTM, but perhaps there should be some changes in AttrDocs.td to document the new support?

[craig.topper]

Add a note to AttrDocs.td

[aaron.ballman]

In D86187#2226621, @craig.topper wrote:

Add a note to AttrDocs.td

Thanks for the docs update, continues to LGTM!

[craig.topper]

clang/include/clang/AST/Attr.h

346

Was it a bug that BranchProtection wasn't in the original code?

[efriedma]

clang/test/Sema/attr-target.c

26

"unsupported tune" seems a little weird. For the command-line flag, we print something like "unknown target CPU 'hiss'"; can we use similar wording here, so it's clear why it's not supported?

[aaron.ballman]

clang/include/clang/AST/Attr.h

346

I just dug out the original review which added it (https://reviews.llvm.org/D68711) and there's no mention of why BranchProtection was missed here, so I think it was just a bug.

[craig.topper]

clang/test/Sema/attr-target.c

26

Sure. Though there is a subtle issue with even the command line version. We print the same message for old CPUs that don't support x86-64. Those cases should probably be "unsupported" rather than "unknown"

[craig.topper]

Refine the diagnostic message

[efriedma]

LGTM

clang/include/clang/AST/Attr.h

 
 static_assert(sizeof(ParamIdx) == sizeof(ParamIdx::SerialType),
               "ParamIdx does not fit its serialization type");
 
 /// Contains information gathered from parsing the contents of TargetAttr.
 struct ParsedTargetAttr {
   std::vector<std::string> Features;
   StringRef Architecture;
+  StringRef Tune;
   StringRef BranchProtection;
   bool DuplicateArchitecture = false;
+  bool DuplicateTune = false;
   bool operator ==(const ParsedTargetAttr &Other) const {
     return DuplicateArchitecture == Other.DuplicateArchitecture &&
-           Architecture == Other.Architecture && Features == Other.Features;
+           DuplicateTune == Other.DuplicateTune &&
+           Architecture == Other.Architecture &&
+           Tune == Other.Tune &&
+           BranchProtection == Other.BranchProtection &&
+           Features == Other.Features;
   }
 };
 
 #include "clang/AST/Attrs.inc"
 
 inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
                                            const Attr *At) {
   DB.AddTaggedVal(reinterpret_cast<intptr_t>(At),

clang/include/clang/Basic/Attr.td

static ParsedTargetAttr parse(StringRef Features) {
 
       // Grab the various features and prepend a "+" to turn on the feature to
       // the backend and add them to our existing set of features.
       for (auto &Feature : AttrFeatures) {
         // Go ahead and trim whitespace rather than either erroring or
         // accepting it weirdly.
         Feature = Feature.trim();
 
-        // We don't support cpu tuning this way currently.
         // TODO: Support the fpmath option. It will require checking
         // overall feature validity for the function with the rest of the
         // attributes on the function.
-        if (Feature.startswith("fpmath=") || Feature.startswith("tune="))
+        if (Feature.startswith("fpmath="))
           continue;
 
         if (Feature.startswith("branch-protection=")) {
           Ret.BranchProtection = Feature.split('=').second.trim();
           continue;
         }
 
         // While we're here iterating check for a different target cpu.
         if (Feature.startswith("arch=")) {
           if (!Ret.Architecture.empty())
             Ret.DuplicateArchitecture = true;
           else
             Ret.Architecture = Feature.split("=").second.trim();
+        } else if (Feature.startswith("tune=")) {
+          if (!Ret.Tune.empty())
+            Ret.DuplicateTune = true;
+          else
+            Ret.Tune = Feature.split("=").second.trim();
         } else if (Feature.startswith("no-"))
           Ret.Features.push_back("-" + Feature.split("-").second.str());
         else
           Ret.Features.push_back("+" + Feature.str());
       }
       return Ret;
     }
   }];

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_attribute_requires_positive_integer : Error<
   "%0 attribute requires a %select{positive|non-negative}1 "
   "integral compile time constant expression">;
 def err_attribute_requires_opencl_version : Error<
   "%0 attribute requires OpenCL version %1%select{| or above}2">;
 def err_invalid_branch_protection_spec : Error<
   "invalid or misplaced branch protection specification '%0'">;
 def warn_unsupported_target_attribute
-    : Warning<"%select{unsupported|duplicate}0%select{| architecture}1 '%2' in"
-              " the 'target' attribute string; 'target' attribute ignored">,
+    : Warning<"%select{unsupported|duplicate}0%select{| architecture| tune}1 "
+              "'%2' in the 'target' attribute string; 'target' attribute "
+              "ignored">,
       InGroup<IgnoredAttributes>;
 def err_attribute_unsupported
     : Error<"%0 attribute is not supported for this target">;
 // The err_*_attribute_argument_not_int are separate because they're used by
 // VerifyIntegerConstantExpression.
 def err_aligned_attribute_argument_not_int : Error<
   "'aligned' attribute requires integer constant">;
 def err_align_value_attribute_argument_not_int : Error<

clang/include/clang/Basic/TargetInfo.h

public:
   /// Fill a SmallVectorImpl with the valid values to setCPU.
   virtual void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const {}
 
   /// brief Determine whether this TargetInfo supports the given CPU name.
   virtual bool isValidCPUName(StringRef Name) const {
     return true;
   }
 
+  /// brief Determine whether this TargetInfo supports tune in target attribute.
+  virtual bool supportsTargetAttributeTune() const {
+    return false;
+  }
+
   /// Use the specified ABI.
   ///
   /// \return False on error (invalid ABI name).
   virtual bool setABI(const std::string &Name) {
     return false;
   }
 
   /// Use the specified unit for FP math.

clang/lib/Basic/Targets/X86.h

StringRef getABI() const override {
     if (getTriple().getArch() == llvm::Triple::x86_64 && SSELevel >= AVX)
       return "avx";
     if (getTriple().getArch() == llvm::Triple::x86 &&
         MMX3DNowLevel == NoMMX3DNow)
       return "no-mmx";
     return "";
   }
 
+  bool supportsTargetAttributeTune() const override {
+    return true;
+  }
+
   bool isValidCPUName(StringRef Name) const override {
     bool Only64Bit = getTriple().getArch() != llvm::Triple::x86;
     return llvm::X86::parseArchX86(Name, Only64Bit) != llvm::X86::CK_None;
   }
 
   void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const override;
 
   bool setCPU(const std::string &Name) override {

clang/lib/CodeGen/CodeGenModule.cpp

for (const llvm::StringMap<bool>::value_type &Entry : FeatureMap)
       Features.push_back((Entry.getValue() ? "+" : "-") + Entry.getKey().str());
 
     // Now add the target-cpu and target-features to the function.
     // While we populated the feature map above, we still need to
     // get and parse the target attribute so we can get the cpu for
     // the function.
     if (TD) {
       ParsedTargetAttr ParsedAttr = TD->parse();
-      if (ParsedAttr.Architecture != "" &&
-          getTarget().isValidCPUName(ParsedAttr.Architecture))
+      if (!ParsedAttr.Architecture.empty() &&
+          getTarget().isValidCPUName(ParsedAttr.Architecture)) {
         TargetCPU = ParsedAttr.Architecture;
+        TuneCPU = ""; // Clear the tune CPU.
+      }
+      if (!ParsedAttr.Tune.empty() &&
+          getTarget().isValidCPUName(ParsedAttr.Tune))
+        TuneCPU = ParsedAttr.Tune;
     }
   } else {
     // Otherwise just add the existing target cpu and target features to the
     // function.
     Features = getTarget().getTargetOpts().Features;
   }
 
-  if (TargetCPU != "") {
+  if (!TargetCPU.empty()) {
     Attrs.addAttribute("target-cpu", TargetCPU);
     AddedAttr = true;
   }
-  if (TuneCPU != "") {
+  if (!TuneCPU.empty()) {
     Attrs.addAttribute("tune-cpu", TuneCPU);
     AddedAttr = true;
   }
   if (!Features.empty()) {
     llvm::sort(Features);
     Attrs.addAttribute("target-features", llvm::join(Features, ","));
     AddedAttr = true;
   }
if (auto *F = dyn_cast<llvm::Function>(GO)) {
 
       llvm::AttrBuilder Attrs;
       if (GetCPUAndFeaturesAttributes(GD, Attrs)) {
         // We know that GetCPUAndFeaturesAttributes will always have the
         // newest set, since it has the newest possible FunctionDecl, so the
         // new ones should replace the old.
         F->removeFnAttr("target-cpu");
         F->removeFnAttr("target-features");
+        F->removeFnAttr("tune-cpu");
         F->addAttributes(llvm::AttributeList::FunctionIndex, Attrs);
       }
     }
 
     if (const auto *CSA = D->getAttr<CodeSegAttr>())
       GO->setSection(CSA->getName());
     else if (const auto *SA = D->getAttr<SectionAttr>())
       GO->setSection(SA->getName());

clang/lib/Sema/SemaDeclAttr.cpp

static void handleCodeSegAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   if (CodeSegAttr *CSA = S.mergeCodeSegAttr(D, AL, Str))
     D->addAttr(CSA);
 }
 
 // Check for things we'd like to warn about. Multiversioning issues are
 // handled later in the process, once we know how many exist.
 bool Sema::checkTargetAttr(SourceLocation LiteralLoc, StringRef AttrStr) {
   enum FirstParam { Unsupported, Duplicate };
-  enum SecondParam { None, Architecture };
-  for (auto Str : {"tune=", "fpmath="})
-    if (AttrStr.find(Str) != StringRef::npos)
-      return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
-             << Unsupported << None << Str;
+  enum SecondParam { None, Architecture, Tune };
+  if (AttrStr.find("fpmath=") != StringRef::npos)
+           << Unsupported << None << "fpmath=";
+
+  // Diagnose use of tune if target doesn't support it.
+  if (!Context.getTargetInfo().supportsTargetAttributeTune() &&
+      AttrStr.find("tune=") != StringRef::npos)
+    return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
+           << Unsupported << None << "tune=";
 
   ParsedTargetAttr ParsedAttrs = TargetAttr::parse(AttrStr);
 
   if (!ParsedAttrs.Architecture.empty() &&
       !Context.getTargetInfo().isValidCPUName(ParsedAttrs.Architecture))
     return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
            << Unsupported << Architecture << ParsedAttrs.Architecture;
 
+  if (!ParsedAttrs.Tune.empty() &&
+      !Context.getTargetInfo().isValidCPUName(ParsedAttrs.Tune))
+    return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
+           << Unsupported << Tune << ParsedAttrs.Tune;
+
   if (ParsedAttrs.DuplicateArchitecture)
     return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
            << Duplicate << None << "arch=";
+  if (ParsedAttrs.DuplicateTune)
+    return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
+           << Duplicate << None << "tune=";
 
   for (const auto &Feature : ParsedAttrs.Features) {
     auto CurFeature = StringRef(Feature).drop_front(); // remove + or -.
     if (!Context.getTargetInfo().isValidFeatureName(CurFeature))
       return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
              << Unsupported << None << CurFeature;
   }
 

clang/test/CodeGen/attr-target-x86.c

-// RUN: %clang_cc1 -triple i686-linux-gnu -target-cpu i686 -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple i686-linux-gnu -target-cpu i686 -tune-cpu i686 -emit-llvm %s -o - | FileCheck %s
 
 int baz(int a) { return 4; }
 
 int __attribute__((target("avx,sse4.2,arch=ivybridge"))) foo(int a) { return 4; }
 
-int __attribute__((target("tune=sandybridge"))) walrus(int a) { return 4; }
 int __attribute__((target("fpmath=387"))) koala(int a) { return 4; }
 
 int __attribute__((target("no-sse2"))) echidna(int a) { return 4; }
 
 int __attribute__((target("sse4"))) panda(int a) { return 4; }
 int __attribute__((target("no-sse4"))) narwhal(int a) { return 4; }
 
 int bar(int a) { return baz(a) + foo(a); }
int useage(void){
   return use_before_def();
 }
 
 // Adding the attribute to a definition does update it in IR.
 int __attribute__((target("arch=lakemont,mmx"))) use_before_def(void) {
   return 5;
 }
 
+int __attribute__((target("tune=sandybridge"))) walrus(int a) { return 4; }
 
 // Check that we emit the additional subtarget and cpu features for foo and not for baz or bar.
 // CHECK: baz{{.*}} #0
 // CHECK: foo{{.*}} #1
-// We ignore the tune attribute so walrus should be identical to baz and bar.
-// CHECK: walrus{{.*}} #0
 // We're currently ignoring the fpmath attribute so koala should be identical to baz and bar.
 // CHECK: koala{{.*}} #0
 // CHECK: echidna{{.*}} #2
 // CHECK: panda{{.*}} #3
 // CHECK: narwhal{{.*}} #4
 // CHECK: bar{{.*}} #0
 // CHECK: qux{{.*}} #1
 // CHECK: qax{{.*}} #5
 // CHECK: qq{{.*}} #6
 // CHECK: lake{{.*}} #7
 // CHECK: use_before_def{{.*}} #7
-// CHECK: #0 = {{.*}}"target-cpu"="i686" "target-features"="+cx8,+x87"
+// CHECK: walrus{{.*}} #8
+// CHECK: #0 = {{.*}}"target-cpu"="i686" "target-features"="+cx8,+x87" "tune-cpu"="i686"
 // CHECK: #1 = {{.*}}"target-cpu"="ivybridge" "target-features"="+avx,+cx16,+cx8,+f16c,+fsgsbase,+fxsr,+mmx,+pclmul,+popcnt,+rdrnd,+sahf,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt"
-// CHECK: #2 = {{.*}}"target-cpu"="i686" "target-features"="+cx8,+x87,-aes,-avx,-avx2,-avx512bf16,-avx512bitalg,-avx512bw,-avx512cd,-avx512dq,-avx512er,-avx512f,-avx512ifma,-avx512pf,-avx512vbmi,-avx512vbmi2,-avx512vl,-avx512vnni,-avx512vp2intersect,-avx512vpopcntdq,-f16c,-fma,-fma4,-gfni,-pclmul,-sha,-sse2,-sse3,-sse4.1,-sse4.2,-sse4a,-ssse3,-vaes,-vpclmulqdq,-xop"
-// CHECK: #3 = {{.*}}"target-cpu"="i686" "target-features"="+cx8,+mmx,+popcnt,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87"
-// CHECK: #4 = {{.*}}"target-cpu"="i686" "target-features"="+cx8,+x87,-avx,-avx2,-avx512bf16,-avx512bitalg,-avx512bw,-avx512cd,-avx512dq,-avx512er,-avx512f,-avx512ifma,-avx512pf,-avx512vbmi,-avx512vbmi2,-avx512vl,-avx512vnni,-avx512vp2intersect,-avx512vpopcntdq,-f16c,-fma,-fma4,-sse4.1,-sse4.2,-vaes,-vpclmulqdq,-xop"
+// CHECK-NOT: tune-cpu
+// CHECK: #2 = {{.*}}"target-cpu"="i686" "target-features"="+cx8,+x87,-aes,-avx,-avx2,-avx512bf16,-avx512bitalg,-avx512bw,-avx512cd,-avx512dq,-avx512er,-avx512f,-avx512ifma,-avx512pf,-avx512vbmi,-avx512vbmi2,-avx512vl,-avx512vnni,-avx512vp2intersect,-avx512vpopcntdq,-f16c,-fma,-fma4,-gfni,-pclmul,-sha,-sse2,-sse3,-sse4.1,-sse4.2,-sse4a,-ssse3,-vaes,-vpclmulqdq,-xop" "tune-cpu"="i686"
+// CHECK: #3 = {{.*}}"target-cpu"="i686" "target-features"="+cx8,+mmx,+popcnt,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87" "tune-cpu"="i686"
+// CHECK: #4 = {{.*}}"target-cpu"="i686" "target-features"="+cx8,+x87,-avx,-avx2,-avx512bf16,-avx512bitalg,-avx512bw,-avx512cd,-avx512dq,-avx512er,-avx512f,-avx512ifma,-avx512pf,-avx512vbmi,-avx512vbmi2,-avx512vl,-avx512vnni,-avx512vp2intersect,-avx512vpopcntdq,-f16c,-fma,-fma4,-sse4.1,-sse4.2,-vaes,-vpclmulqdq,-xop" "tune-cpu"="i686"
 // CHECK: #5 = {{.*}}"target-cpu"="ivybridge" "target-features"="+avx,+cx16,+cx8,+f16c,+fsgsbase,+fxsr,+mmx,+pclmul,+popcnt,+rdrnd,+sahf,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87,+xsave,+xsaveopt,-aes,-vaes"
+// CHECK-NOT: tune-cpu
 // CHECK: #6 = {{.*}}"target-cpu"="i686" "target-features"="+cx8,+x87,-3dnow,-3dnowa,-mmx"
 // CHECK: #7 = {{.*}}"target-cpu"="lakemont" "target-features"="+cx8,+mmx"
+// CHECK-NOT: tune-cpu
+// CHECK: #8 = {{.*}}"target-cpu"="i686" "target-features"="+cx8,+x87" "tune-cpu"="sandybridge"

clang/test/Sema/attr-target.c

 // RUN: %clang_cc1 -triple x86_64-linux-gnu  -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple aarch64-linux-gnu  -fsyntax-only -verify %s
+
+#ifdef __x86_64__
 
 int __attribute__((target("avx,sse4.2,arch=ivybridge"))) foo() { return 4; }
 //expected-error@+1 {{'target' attribute takes one argument}}
 int __attribute__((target())) bar() { return 4; }
-//expected-warning@+1 {{unsupported 'tune=' in the 'target' attribute string; 'target' attribute ignored}}
+// no warning, tune is supported for x86
 int __attribute__((target("tune=sandybridge"))) baz() { return 4; }
 //expected-warning@+1 {{unsupported 'fpmath=' in the 'target' attribute string; 'target' attribute ignored}}
 int __attribute__((target("fpmath=387"))) walrus() { return 4; }
 //expected-warning@+1 {{unsupported architecture 'hiss' in the 'target' attribute string; 'target' attribute ignored}}
 int __attribute__((target("avx,sse4.2,arch=hiss"))) meow() {  return 4; }
 //expected-warning@+1 {{unsupported 'woof' in the 'target' attribute string; 'target' attribute ignored}}
 int __attribute__((target("woof"))) bark() {  return 4; }
 // no warning, same as saying 'nothing'.
 int __attribute__((target("arch="))) turtle() { return 4; }
 //expected-warning@+1 {{unsupported architecture 'hiss' in the 'target' attribute string; 'target' attribute ignored}}
 int __attribute__((target("arch=hiss,arch=woof"))) pine_tree() { return 4; }
 //expected-warning@+1 {{duplicate 'arch=' in the 'target' attribute string; 'target' attribute ignored}}
 int __attribute__((target("arch=ivybridge,arch=haswell"))) oak_tree() { return 4; }
 //expected-warning@+1 {{unsupported 'branch-protection' in the 'target' attribute string; 'target' attribute ignored}}
 int __attribute__((target("branch-protection=none"))) birch_tree() { return 5; }
+//expected-warning@+1 {{unsupported tune 'hiss' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("tune=hiss,tune=woof"))) apple_tree() { return 4; }
+
+#else
+
+// tune is not supported by other targets.
+//expected-warning@+1 {{unsupported 'tune=' in the 'target' attribute string; 'target' attribute ignored}}
+int __attribute__((target("tune=hiss"))) baz() { return 4; }
 
+#endif