Add SEMA checking to attribute 'target'

[erichkeane]

Summary

Attribute 'target' can take either features or architectures. This patch accomplishes this in a couple of ways:

1- It adds support to Targets.cpp for "isValidCPUName" and "isValidFeatureName". HOWEVER, the latter is only implemented for X86. I'm hoping to delay implementation of those until later (or get help on that).
2- Cleans up the 'target' entry in Attr.td to use a struct instead of a 'Pair', so that more data can be included.
3- Validates the attribute sufficiently.

Note that we currently do not have multi-versioning support (that is to come in a future patch) and SemaDeclAttr.cpp lacks knowledge of whether this is a multi-version, so this validation doesn't cover the multi-versioning case.

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

2389–2397

All of these (including the one you didn't have to touch) should be using "ignoring" instead of "Ignoring".

2395

This name should have something to do with attributes. Might make sense to combine with warn_unsupported_target_attribute using a %select given the similarity in use case.

lib/Basic/Targets.cpp

1015–1018

I think this comment should be hoisted up to isValidCPUName().

6416

It's rather strange that this doesn't actually set the CPU, but I can't see that the original code did either, so maybe it's fine?

9617

You can drop the this, and I would put the CPU = Name onto its own line (not certain if that's a personal preference or a general community style).

lib/Sema/SemaDeclAttr.cpp

3004

Missing a space after the if.

3005

Should this be "arch=" to be consistent with the previous diagnostic on line 3001?

3007

Is the attribute valid when the architecture is empty?

3012

Can this be made const auto &?

3013

Insert a space before the comment.

test/Sema/attr-target.c

9

Please add a test case where there's a duplicate arch and an empty arch.

[erichkeane]

Thanks for the feedback Aaron. This should fix everything you had an issue with.

-Erich

[aaron.ballman]

A few small nits, but otherwise LGTM

lib/Sema/SemaDeclAttr.cpp

2999–3000

I don't think this comment is needed (the code is pretty obvious), so I'd just drop it entirely.

3001–3002

enumerator casing should be Unsupported, Duplicate, Architecture.

[erichkeane]

Thanks Aaron! Fixed those things and will be submitting soon. Also ran format on the function in SemaDeclAttr. For some reason it didn't right the first time.

[echristo]

If you wouldn't mind I'd like to see this separated out a bit -
a) I think we can make the attribute info a struct rather than a pair as a simple change first
b) setCPU split
c) let's see where we are after that?

Also the description makes it sound like you're adding support for the feature rather than fixing the terrible error diagnostics - or at least to my early morning brain. :)

Also, one inline comment so far.

Thanks!

-eric

test/Sema/attr-target.c

11

Precedence issue maybe, but I'd have expected this to error on the architecture name rather than the duplicate?

[erichkeane]

In D35519#813082, @echristo wrote:

If you wouldn't mind I'd like to see this separated out a bit -
a) I think we can make the attribute info a struct rather than a pair as a simple change first
b) setCPU split
c) let's see where we are after that?

Also the description makes it sound like you're adding support for the feature rather than fixing the terrible error diagnostics - or at least to my early morning brain. :)

Also, one inline comment so far.

Thanks!

-eric

Caught me just in time :)

Sure, I can split this into a couple of patches. I'll abandon this one and go from there.

[erichkeane]

Going to split this into a few patches as Eric requested.

include/clang/Basic/Attr.td

 }
 
 def Target : InheritableAttr {
   let Spellings = [GCC<"target">];
   let Args = [StringArgument<"featuresStr">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [TargetDocs];
   let AdditionalMembers = [{
-    typedef std::pair<std::vector<std::string>, StringRef> ParsedTargetAttr;
+    struct ParsedTargetAttr {
+      std::vector<std::string> Features;
+      StringRef Architecture;
+      bool DuplicateArchitecture = false;
+    };
     ParsedTargetAttr parse() const {
+      return parse(getFeaturesStr());
+    }
+    static ParsedTargetAttr parse(StringRef Features) {
       ParsedTargetAttr Ret;
       SmallVector<StringRef, 1> AttrFeatures;
-      getFeaturesStr().split(AttrFeatures, ",");
+      Features.split(AttrFeatures, ",");
 
       // 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="))
     continue;
 
         // While we're here iterating check for a different target cpu.
-        if (Feature.startswith("arch="))
-          Ret.second = Feature.split("=").second.trim();
-        else if (Feature.startswith("no-"))
-          Ret.first.push_back("-" + Feature.split("-").second.str());
+        if (Feature.startswith("arch=")) {
+          if (!Ret.Architecture.empty())
+            Ret.DuplicateArchitecture = true;
+          else 
+            Ret.Architecture = Feature.split("=").second.trim();
+        } else if (Feature.startswith("no-"))
+          Ret.Features.push_back("-" + Feature.split("-").second.str());
         else
-          Ret.first.push_back("+" + Feature.str());
+          Ret.Features.push_back("+" + Feature.str());
       }
       return Ret;
     }
   }];
 }
 
 def TransparentUnion : InheritableAttr {
   let Spellings = [GCC<"transparent_union">];

include/clang/Basic/DiagnosticSemaKinds.td

def err_attribute_too_few_arguments : Error<
   "%0 attribute takes at least %1 argument%s1">;
 def err_attribute_invalid_vector_type : Error<"invalid vector element type %0">;
 def err_attribute_bad_neon_vector_size : Error<
   "Neon vector size must be 64 or 128 bits">;
 def err_attribute_requires_positive_integer : Error<
   "%0 attribute requires a positive integral compile time constant expression">;
 def err_attribute_requires_opencl_version : Error<
   "%0 attribute requires OpenCL version %1%select{| or above}2">;
 def warn_unsupported_target_attribute
     : Warning<"Ignoring unsupported '%0' in the target attribute string">,
     InGroup<IgnoredAttributes>;
+def warn_duplicate_target_attribute
+    : Warning<"Ignoring duplicate '%0' in the target attribute string">,
+    InGroup<IgnoredAttributes>;
+def warn_unsupported_target_architecture
+    : Warning<"Ignoring unsupported architecture '%0' in the target attribute "
+              "string">,
+      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<
   "'align_value' attribute requires integer constant">;

include/clang/Basic/TargetInfo.h

public:
 
   /// \brief Target the specified CPU.
   ///
   /// \return  False on error (invalid CPU name).
   virtual bool setCPU(const std::string &Name) {
     return false;
   }
 
+  /// brief Determine whether this TargetInfo supports the given CPU name.
+  virtual bool isValidCPUName(StringRef Name) const {
+    return false;
+  }
+
   /// \brief Use the specified ABI.
   ///
   /// \return False on error (invalid ABI name).
   virtual bool setABI(const std::string &Name) {
     return false;
   }
 
   /// \brief Use the specified unit for FP math.
   ///
   /// \return False on error (invalid unit name).
   virtual bool setFPMath(StringRef Name) {
     return false;
   }
 
   /// \brief Enable or disable a specific target feature;
   /// the feature name must be valid.
   virtual void setFeatureEnabled(llvm::StringMap<bool> &Features,
                                  StringRef Name,
                                  bool Enabled) const {
     Features[Name] = Enabled;
   }
 
+  /// \brief Determine whether this TargetInfo supports the given feature.
+  virtual bool isValidFeatureName(StringRef Feature) const {
+    return false;
+  }
+
   /// \brief Perform initialization based on the user configured
   /// set of features (e.g., +sse4).
   ///
   /// The list is guaranteed to have at most one entry per feature.
   ///
   /// The target may modify the features list, to change which options are
   /// passed onwards to the backend.
   /// FIXME: This part should be fixed so that we can change handleTargetFeatures

include/clang/Sema/Sema.h

public:
   bool CheckCallingConvAttr(const AttributeList &attr, CallingConv &CC,
                             const FunctionDecl *FD = nullptr);
   bool CheckNoReturnAttr(const AttributeList &attr);
   bool CheckNoCallerSavedRegsAttr(const AttributeList &attr);
   bool checkStringLiteralArgumentAttr(const AttributeList &Attr,
                                       unsigned ArgNum, StringRef &Str,
                                       SourceLocation *ArgLocation = nullptr);
   bool checkSectionName(SourceLocation LiteralLoc, StringRef Str);
-  void checkTargetAttr(SourceLocation LiteralLoc, StringRef Str);
+  bool checkTargetAttr(SourceLocation LiteralLoc, StringRef Str);
   bool checkMSInheritanceAttrOnDefinition(
       CXXRecordDecl *RD, SourceRange Range, bool BestCase,
       MSInheritanceAttr::Spelling SemanticSpelling);
 
   void CheckAlignasUnderalignment(Decl *D);
 
   /// Adjust the calling convention of a method to be the ABI default if it
   /// wasn't specified explicitly.  This handles method types formed from

lib/Basic/Targets.cpp

public:
 
   // Set the language option for altivec based on our value.
   void adjust(LangOptions &Opts) override {
     if (HasAltivec)
       Opts.AltiVec = 1;
     TargetInfo::adjust(Opts);
   }
 
-  // Note: GCC recognizes the following additional cpus:
-  //  401, 403, 405, 405fp, 440fp, 464, 464fp, 476, 476fp, 505, 740, 801,
-  //  821, 823, 8540, 8548, e300c2, e300c3, e500mc64, e6500, 860, cell,
-  //  titan, rs64.
-  bool setCPU(const std::string &Name) override {
-    bool CPUKnown = llvm::StringSwitch<bool>(Name)
+  bool isValidCPUName(StringRef Name) const override {
+    return llvm::StringSwitch<bool>(Name)
       .Case("generic", true)
       .Case("440", true)
       .Case("450", true)
       .Case("601", true)
       .Case("602", true)
       .Case("603", true)
       .Case("603e", true)
       .Case("603ev", true)
return llvm::StringSwitch<bool>(Name)
       .Case("pwr9", true)
       .Case("powerpc", true)
       .Case("ppc", true)
       .Case("powerpc64", true)
       .Case("ppc64", true)
       .Case("powerpc64le", true)
       .Case("ppc64le", true)
       .Default(false);
+  }
 
+  // Note: GCC recognizes the following additional cpus:
+  //  401, 403, 405, 405fp, 440fp, 464, 464fp, 476, 476fp, 505, 740, 801,
+  //  821, 823, 8540, 8548, e300c2, e300c3, e500mc64, e6500, 860, cell,
+  //  titan, rs64.
+  bool setCPU(const std::string &Name) override {
+    bool CPUKnown = isValidCPUName(Name);
     if (CPUKnown)
       CPU = Name;
-
     return CPUKnown;
   }
 
 
   StringRef getABI() const override { return ABI; }
 
   ArrayRef<Builtin::Info> getTargetBuiltins() const override {
     return llvm::makeArrayRef(BuiltinInfo,
public:
   const char *getClobbers() const override {
     // FIXME: Is this really right?
     return "";
   }
   BuiltinVaListKind getBuiltinVaListKind() const override {
     // FIXME: implement
     return TargetInfo::CharPtrBuiltinVaList;
   }
+  bool isValidCPUName(StringRef Name) const override {
+    return StringToCudaArch(Name) != CudaArch::UNKNOWN;
+  }
   bool setCPU(const std::string &Name) override {
     GPU = StringToCudaArch(Name);
     return GPU != CudaArch::UNKNOWN;
   }
   void setSupportedOpenCLOpts() override {
     auto &Opts = getSupportedOpenCLOpts();
     Opts.support("cl_clang_storage_class_specifiers");
     Opts.support("cl_khr_gl_sharing");
return llvm::StringSwitch<GPUKind>(Name)
       .Case("gfx803",    GK_GFX8)
       .Case("gfx804",    GK_GFX8)
       .Case("gfx810",    GK_GFX8)
       .Case("gfx900",    GK_GFX9)
       .Case("gfx901",    GK_GFX9)
       .Default(GK_NONE);
   }
 
+  bool isValidCPUName(StringRef Name) const override {
+    if (getTriple().getArch() == llvm::Triple::amdgcn)
+      return GK_NONE !=  parseAMDGCNName(Name);
+    else
+      return GK_NONE !=  parseR600Name(Name);
+  }
+
   bool setCPU(const std::string &Name) override {
     if (getTriple().getArch() == llvm::Triple::amdgcn)
       GPU = parseAMDGCNName(Name);
     else
       GPU = parseR600Name(Name);
 
     return GPU != GK_NONE;
   }
enum CPUKind {
 
     /// \name Geode
     /// Geode processors.
     //@{
     CK_Geode
     //@}
   } CPU = CK_Generic;
 
+  bool checkCPUKind(CPUKind Kind) const {
+    // Perform any per-CPU checks necessary to determine if this CPU is
+    // acceptable.
+    // FIXME: This results in terrible diagnostics. Clang just says the CPU is
+    // invalid without explaining *why*.
+    switch (Kind) {
+    case CK_Generic:
+      // No processor selected!
+      return false;
+
+    case CK_i386:
+    case CK_i486:
+    case CK_WinChipC6:
+    case CK_WinChip2:
+    case CK_C3:
+    case CK_i586:
+    case CK_Pentium:
+    case CK_PentiumMMX:
+    case CK_i686:
+    case CK_PentiumPro:
+    case CK_Pentium2:
+    case CK_Pentium3:
+    case CK_Pentium3M:
+    case CK_PentiumM:
+    case CK_Yonah:
+    case CK_C3_2:
+    case CK_Pentium4:
+    case CK_Pentium4M:
+    case CK_Lakemont:
+    case CK_Prescott:
+    case CK_K6:
+    case CK_K6_2:
+    case CK_K6_3:
+    case CK_Athlon:
+    case CK_AthlonThunderbird:
+    case CK_Athlon4:
+    case CK_AthlonXP:
+    case CK_AthlonMP:
+    case CK_Geode:
+      // Only accept certain architectures when compiling in 32-bit mode.
+      if (getTriple().getArch() != llvm::Triple::x86)
+        return false;
+
+      // Fallthrough
+    case CK_Nocona:
+    case CK_Core2:
+    case CK_Penryn:
+    case CK_Bonnell:
+    case CK_Silvermont:
+    case CK_Goldmont:
+    case CK_Nehalem:
+    case CK_Westmere:
+    case CK_SandyBridge:
+    case CK_IvyBridge:
+    case CK_Haswell:
+    case CK_Broadwell:
+    case CK_SkylakeClient:
+    case CK_SkylakeServer:
+    case CK_Cannonlake:
+    case CK_KNL:
+    case CK_Athlon64:
+    case CK_Athlon64SSE3:
+    case CK_AthlonFX:
+    case CK_K8:
+    case CK_K8SSE3:
+    case CK_Opteron:
+    case CK_OpteronSSE3:
+    case CK_AMDFAM10:
+    case CK_BTVER1:
+    case CK_BTVER2:
+    case CK_BDVER1:
+    case CK_BDVER2:
+    case CK_BDVER3:
+    case CK_BDVER4:
+    case CK_ZNVER1:
+    case CK_x86_64:
+      return true;
+    }
+    llvm_unreachable("Unhandled CPU kind");
+  }
+
   CPUKind getCPUKind(StringRef CPU) const {
     return llvm::StringSwitch<CPUKind>(CPU)
         .Case("i386", CK_i386)
         .Case("i486", CK_i486)
         .Case("winchip-c6", CK_WinChipC6)
         .Case("winchip2", CK_WinChip2)
         .Case("c3", CK_C3)
         .Case("i586", CK_i586)
public:
   // This exists purely to cut down on the number of virtual calls in
   // initFeatureMap which calls this repeatedly.
   static void setFeatureEnabledImpl(llvm::StringMap<bool> &Features,
                                     StringRef Name, bool Enabled);
   bool
   initFeatureMap(llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags,
                  StringRef CPU,
                  const std::vector<std::string> &FeaturesVec) const override;
+  bool isValidFeatureName(StringRef Name) const override;
   bool hasFeature(StringRef Feature) const override;
   bool handleTargetFeatures(std::vector<std::string> &Features,
                             DiagnosticsEngine &Diags) override;
   StringRef getABI() const override {
     if (getTriple().getArch() == llvm::Triple::x86_64 && SSELevel >= AVX512F)
       return "avx512";
     if (getTriple().getArch() == llvm::Triple::x86_64 && SSELevel >= AVX)
       return "avx";
     if (getTriple().getArch() == llvm::Triple::x86 &&
              MMX3DNowLevel == NoMMX3DNow)
       return "no-mmx";
     return "";
   }
-  bool setCPU(const std::string &Name) override {
-    CPU = getCPUKind(Name);
-
-    // Perform any per-CPU checks necessary to determine if this CPU is
-    // acceptable.
-    // FIXME: This results in terrible diagnostics. Clang just says the CPU is
-    // invalid without explaining *why*.
-    switch (CPU) {
-    case CK_Generic:
-      // No processor selected!
-      return false;
 
-    case CK_i386:
-    case CK_i486:
-    case CK_WinChipC6:
-    case CK_WinChip2:
-    case CK_C3:
-    case CK_i586:
-    case CK_Pentium:
-    case CK_PentiumMMX:
-    case CK_i686:
-    case CK_PentiumPro:
-    case CK_Pentium2:
-    case CK_Pentium3:
-    case CK_Pentium3M:
-    case CK_PentiumM:
-    case CK_Yonah:
-    case CK_C3_2:
-    case CK_Pentium4:
-    case CK_Pentium4M:
-    case CK_Lakemont:
-    case CK_Prescott:
-    case CK_K6:
-    case CK_K6_2:
-    case CK_K6_3:
-    case CK_Athlon:
-    case CK_AthlonThunderbird:
-    case CK_Athlon4:
-    case CK_AthlonXP:
-    case CK_AthlonMP:
-    case CK_Geode:
-      // Only accept certain architectures when compiling in 32-bit mode.
-      if (getTriple().getArch() != llvm::Triple::x86)
-        return false;
-
-      // Fallthrough
-    case CK_Nocona:
-    case CK_Core2:
-    case CK_Penryn:
-    case CK_Bonnell:
-    case CK_Silvermont:
-    case CK_Goldmont:
-    case CK_Nehalem:
-    case CK_Westmere:
-    case CK_SandyBridge:
-    case CK_IvyBridge:
-    case CK_Haswell:
-    case CK_Broadwell:
-    case CK_SkylakeClient:
-    case CK_SkylakeServer:
-    case CK_Cannonlake:
-    case CK_KNL:
-    case CK_Athlon64:
-    case CK_Athlon64SSE3:
-    case CK_AthlonFX:
-    case CK_K8:
-    case CK_K8SSE3:
-    case CK_Opteron:
-    case CK_OpteronSSE3:
-    case CK_AMDFAM10:
-    case CK_BTVER1:
-    case CK_BTVER2:
-    case CK_BDVER1:
-    case CK_BDVER2:
-    case CK_BDVER3:
-    case CK_BDVER4:
-    case CK_ZNVER1:
-    case CK_x86_64:
-      return true;
-    }
-    llvm_unreachable("Unhandled CPU kind");
+  bool isValidCPUName(StringRef Name) const override {
+    return checkCPUKind(getCPUKind(Name));
+
+  bool setCPU(const std::string &Name) override {
+    return checkCPUKind(CPU = getCPUKind(Name));
   }
 
   bool setFPMath(StringRef Name) override;
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override {
     // Most of the non-ARM calling conventions are i386 conventions.
     switch (CC) {
     case CC_X86ThisCall:
void X86TargetInfo::getTargetDefines(const LangOptions &Opts,
   }
   if (CPU >= CK_i586)
     Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8");
 
   if (HasFloat128)
     Builder.defineMacro("__SIZEOF_FLOAT128__", "16");
 }
 
+bool X86TargetInfo::isValidFeatureName(StringRef Name) const {
+  return llvm::StringSwitch<bool>(Name)
+      .Case("aes", true)
+      .Case("avx", true)
+      .Case("avx2", true)
+      .Case("avx512f", true)
+      .Case("avx512cd", true)
+      .Case("avx512vpopcntdq", true)
+      .Case("avx512er", true)
+      .Case("avx512pf", true)
+      .Case("avx512dq", true)
+      .Case("avx512bw", true)
+      .Case("avx512vl", true)
+      .Case("avx512vbmi", true)
+      .Case("avx512ifma", true)
+      .Case("bmi", true)
+      .Case("bmi2", true)
+      .Case("clflushopt", true)
+      .Case("clwb", true)
+      .Case("clzero", true)
+      .Case("cx16", true)
+      .Case("f16c", true)
+      .Case("fma", true)
+      .Case("fma4", true)
+      .Case("fsgsbase", true)
+      .Case("fxsr", true)
+      .Case("lwp", true)
+      .Case("lzcnt", true)
+      .Case("mm3dnow", true)
+      .Case("mm3dnowa", true)
+      .Case("mmx", true)
+      .Case("movbe", true)
+      .Case("mpx", true)
+      .Case("pclmul", true)
+      .Case("pku", true)
+      .Case("popcnt", true)
+      .Case("prefetchwt1", true)
+      .Case("prfchw", true)
+      .Case("rdrnd", true)
+      .Case("rdseed", true)
+      .Case("rtm", true)
+      .Case("sgx", true)
+      .Case("sha", true)
+      .Case("sse", true)
+      .Case("sse2", true)
+      .Case("sse3", true)
+      .Case("ssse3", true)
+      .Case("sse4.1", true)
+      .Case("sse4.2", true)
+      .Case("sse4a", true)
+      .Case("tbm", true)
+      .Case("x86", true)
+      .Case("x86_32", true)
+      .Case("x86_64", true)
+      .Case("xop", true)
+      .Case("xsave", true)
+      .Case("xsavec", true)
+      .Case("xsaves", true)
+      .Case("xsaveopt", true)
+      .Default(false);
+}
+
 bool X86TargetInfo::hasFeature(StringRef Feature) const {
   return llvm::StringSwitch<bool>(Feature)
       .Case("aes", HasAES)
       .Case("avx", SSELevel >= AVX)
       .Case("avx2", SSELevel >= AVX2)
       .Case("avx512f", SSELevel >= AVX512F)
       .Case("avx512cd", HasAVX512CD)
       .Case("avx512vpopcntdq", HasAVX512VPOPCNTDQ)
return llvm::StringSwitch<bool>(Feature)
       .Case("clwb", HasCLWB)
       .Case("clzero", HasCLZERO)
       .Case("cx16", HasCX16)
       .Case("f16c", HasF16C)
       .Case("fma", HasFMA)
       .Case("fma4", XOPLevel >= FMA4)
       .Case("fsgsbase", HasFSGSBASE)
       .Case("fxsr", HasFXSR)
+      .Case("lwp", HasLWP)
       .Case("lzcnt", HasLZCNT)
       .Case("mm3dnow", MMX3DNowLevel >= AMD3DNow)
       .Case("mm3dnowa", MMX3DNowLevel >= AMD3DNowAthlon)
       .Case("mmx", MMX3DNowLevel >= MMX)
       .Case("movbe", HasMOVBE)
       .Case("mpx", HasMPX)
       .Case("pclmul", HasPCLMUL)
       .Case("pku", HasPKU)
       .Case("popcnt", HasPOPCNT)
       .Case("prefetchwt1", HasPREFETCHWT1)
       .Case("prfchw", HasPRFCHW)
       .Case("rdrnd", HasRDRND)
       .Case("rdseed", HasRDSEED)
       .Case("rtm", HasRTM)
       .Case("sgx", HasSGX)
       .Case("sha", HasSHA)
       .Case("sse", SSELevel >= SSE1)
       .Case("sse2", SSELevel >= SSE2)
       .Case("sse3", SSELevel >= SSE3)
       .Case("ssse3", SSELevel >= SSSE3)
       .Case("sse4.1", SSELevel >= SSE41)
       .Case("sse4.2", SSELevel >= SSE42)
       .Case("sse4a", XOPLevel >= SSE4A)
       .Case("tbm", HasTBM)
-      .Case("lwp", HasLWP)
       .Case("x86", true)
       .Case("x86_32", getTriple().getArch() == llvm::Triple::x86)
       .Case("x86_64", getTriple().getArch() == llvm::Triple::x86_64)
       .Case("xop", XOPLevel >= XOP)
       .Case("xsave", HasXSAVE)
       .Case("xsavec", HasXSAVEC)
       .Case("xsaves", HasXSAVES)
       .Case("xsaveopt", HasXSAVEOPT)
return llvm::StringSwitch<bool>(Feature)
         .Case("thumb", isThumb())
         .Case("neon", (FPU & NeonFPU) && !SoftFloat)
         .Case("vfp", FPU && !SoftFloat)
         .Case("hwdiv", HWDiv & HWDivThumb)
         .Case("hwdiv-arm", HWDiv & HWDivARM)
         .Default(false);
   }
 
+  bool isValidCPUName(StringRef Name) const override {
+    return Name == "generic" ||
+           llvm::ARM::parseCPUArch(Name) != llvm::ARM::AK_INVALID;
+  }
+
   bool setCPU(const std::string &Name) override {
     if (Name != "generic")
       setArchInfo(llvm::ARM::parseCPUArch(Name));
 
     if (ArchKind == llvm::ARM::AK_INVALID)
       return false;
     setAtomic();
     CPU = Name;
public:
   bool setABI(const std::string &Name) override {
     if (Name != "aapcs" && Name != "darwinpcs")
       return false;
 
     ABI = Name;
     return true;
   }
 
-  bool setCPU(const std::string &Name) override {
+  bool isValidCPUName(StringRef Name) const override {
     return Name == "generic" ||
            llvm::AArch64::parseCPUArch(Name) !=
            static_cast<unsigned>(llvm::AArch64::ArchKind::AK_INVALID);
   }
 
+  bool setCPU(const std::string &Name) override {
+    return isValidCPUName(Name);
+  }
+
   void getTargetDefinesARMV81A(const LangOptions &Opts,
                         MacroBuilder &Builder) const {
     Builder.defineMacro("__ARM_FEATURE_QRDMX", "1");
   }
 
   void getTargetDefinesARMV82A(const LangOptions &Opts,
                         MacroBuilder &Builder) const {
     // Also include the ARMv8.1 defines
return llvm::StringSwitch<const char*>(Name)
       .Case("hexagonv4", "4")
       .Case("hexagonv5", "5")
       .Case("hexagonv55", "55")
       .Case("hexagonv60", "60")
       .Case("hexagonv62", "62")
       .Default(nullptr);
   }
 
+  bool isValidCPUName(StringRef Name) const override {
+    return getHexagonCPUSuffix(Name);
+  }
+
   bool setCPU(const std::string &Name) override {
-    if (!getHexagonCPUSuffix(Name))
+    if (!isValidCPUName(Name))
       return false;
     CPU = Name;
     return true;
   }
 
   int getEHDataRegisterNumber(unsigned RegNo) const override {
     return RegNo < 2 ? RegNo : -1;
   }
void getTargetDefines(const LangOptions &Opts,
     case CK_V11:
       Builder.defineMacro("__LANAI_V11__");
       break;
     case CK_NONE:
       llvm_unreachable("Unhandled target CPU");
     }
   }
 
+  bool isValidCPUName(StringRef Name) const override {
+    return llvm::StringSwitch<bool>(Name)
+              .Case("v11", true)
+              .Default(false);
+  }
+
   bool setCPU(const std::string &Name) override {
     CPU = llvm::StringSwitch<CPUKind>(Name)
               .Case("v11", CK_V11)
               .Default(CK_NONE);
 
     return CPU != CK_NONE;
   }
 
return llvm::StringSwitch<CPUKind>(Name)
         .Case("leon3", CK_LEON3)
         .Case("ut699", CK_LEON3_UT699)
         .Case("gr712rc", CK_LEON3_GR712RC)
         .Case("leon4", CK_LEON4)
         .Case("gr740", CK_LEON4_GR740)
         .Default(CK_GENERIC);
   }
 
+  bool isValidCPUName(StringRef Name) const override {
+   return getCPUKind(Name) != CK_GENERIC; 
+  }
+
   bool setCPU(const std::string &Name) override {
     CPU = getCPUKind(Name);
     return CPU != CK_GENERIC;
   }
 };
 
 const char * const SparcTargetInfo::GCCRegNames[] = {
   "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
void getTargetDefines(const LangOptions &Opts,
     // Solaris doesn't need these variants, but the BSDs do.
     if (getTriple().getOS() != llvm::Triple::Solaris) {
       Builder.defineMacro("__sparc64__");
       Builder.defineMacro("__sparc_v9__");
       Builder.defineMacro("__sparcv9__");
     }
   }
 
+  bool isValidCPUName(StringRef Name) const override {
+    return getCPUGeneration(SparcTargetInfo::getCPUKind(Name)) == CG_V9;
+  }
+
   bool setCPU(const std::string &Name) override {
     if (!SparcTargetInfo::setCPU(Name))
       return false;
     return getCPUGeneration(CPU) == CG_V9;
   }
 };
 
 class SystemZTargetInfo : public TargetInfo {
int getISARevision(const StringRef &Name) const {
     return llvm::StringSwitch<int>(Name)
       .Cases("arch8", "z10", 8)
       .Cases("arch9", "z196", 9)
       .Cases("arch10", "zEC12", 10)
       .Cases("arch11", "z13", 11)
       .Cases("arch12", "z14", 12)
       .Default(-1);
   }
+
+  bool isValidCPUName(StringRef Name) const override {
+    return getISARevision(Name) != -1;
+  }
+
   bool setCPU(const std::string &Name) override {
     CPU = Name;
     ISARevision = getISARevision(CPU);
     return ISARevision != -1;
   }
   bool
   initFeatureMap(llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags,
                  StringRef CPU,
public:
   bool setABI(const std::string &Name) override {
     if (Name == "o32" || Name == "eabi") {
       ABI = Name;
       return true;
     }
     return false;
   }
 
+  bool isValidCPUName(StringRef Name) const override {
+    return Name == "nios2r1" || Name == "nios2r2";
+  }
+
   bool setCPU(const std::string &Name) override {
-    if (Name == "nios2r1" || Name == "nios2r2") {
+    if (isValidCPUName(Name)) {
       CPU = Name;
       return true;
     }
     return false;
   }
 
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override {
void setN32ABITypes() {
     Int64Type = SignedLongLong;
     IntMaxType = Int64Type;
     LongWidth = LongAlign = 32;
     PointerWidth = PointerAlign = 32;
     PtrDiffType = SignedInt;
     SizeType = UnsignedInt;
   }
 
-  bool setCPU(const std::string &Name) override {
-    CPU = Name;
+  bool isValidCPUName(StringRef Name) const override {
     return llvm::StringSwitch<bool>(Name)
         .Case("mips1", true)
         .Case("mips2", true)
         .Case("mips3", true)
         .Case("mips4", true)
         .Case("mips5", true)
         .Case("mips32", true)
         .Case("mips32r2", true)
         .Case("mips32r3", true)
         .Case("mips32r5", true)
         .Case("mips32r6", true)
         .Case("mips64", true)
         .Case("mips64r2", true)
         .Case("mips64r3", true)
         .Case("mips64r5", true)
         .Case("mips64r6", true)
         .Case("octeon", true)
         .Case("p5600", true)
         .Default(false);
   }
+
+  bool setCPU(const std::string &Name) override {
+    CPU = Name;
+    return isValidCPUName(Name);
+  }
+
   const std::string& getCPU() const { return CPU; }
   bool
   initFeatureMap(llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags,
                  StringRef CPU,
                  const std::vector<std::string> &FeaturesVec) const override {
     if (CPU.empty())
       CPU = getCPU();
     if (CPU == "octeon")
for (const auto &Feature : Features) {
       }
 
       Diags.Report(diag::err_opt_not_valid_with_opt) << Feature
                                                      << "-target-feature";
       return false;
     }
     return true;
   }
-  bool setCPU(const std::string &Name) final {
+  bool isValidCPUName(StringRef Name) const final {
     return llvm::StringSwitch<bool>(Name)
               .Case("mvp",           true)
               .Case("bleeding-edge", true)
               .Case("generic",       true)
               .Default(false);
   }
+  bool setCPU(const std::string &Name) final {
+    return isValidCPUName(Name);
+  }
   ArrayRef<Builtin::Info> getTargetBuiltins() const final {
     return llvm::makeArrayRef(BuiltinInfo,
                    clang::WebAssembly::LastTSBuiltin - Builtin::FirstTSBuiltin);
   }
   BuiltinVaListKind getBuiltinVaListKind() const final {
     return VoidPtrBuiltinVaList;
   }
   ArrayRef<const char *> getGCCRegNames() const final {
public:
   IntType getLeastIntTypeByWidth(unsigned BitWidth,
                                  bool IsSigned) const final {
     // AVR uses int for int_least16_t and int_fast16_t.
     return BitWidth == 16
                ? (IsSigned ? SignedInt : UnsignedInt)
                : TargetInfo::getLeastIntTypeByWidth(BitWidth, IsSigned);
   }
 
-  bool setCPU(const std::string &Name) override {
+  bool isValidCPUName(StringRef Name) const override {
     bool IsFamily = llvm::StringSwitch<bool>(Name)
       .Case("avr1", true)
       .Case("avr2", true)
       .Case("avr25", true)
       .Case("avr3", true)
       .Case("avr31", true)
       .Case("avr35", true)
       .Case("avr4", true)
       .Case("avr5", true)
       .Case("avr51", true)
       .Case("avr6", true)
       .Case("avrxmega1", true)
       .Case("avrxmega2", true)
       .Case("avrxmega3", true)
       .Case("avrxmega4", true)
       .Case("avrxmega5", true)
       .Case("avrxmega6", true)
       .Case("avrxmega7", true)
       .Case("avrtiny", true)
       .Default(false);
 
-    if (IsFamily) this->CPU = Name;
-
     bool IsMCU = std::find_if(AVRMcus.begin(), AVRMcus.end(),
       [&](const MCUInfo &Info) { return Info.Name == Name; }) != AVRMcus.end();
-
-    if (IsMCU) this->CPU = Name;
-
     return IsFamily || IsMCU;
   }
 
+  bool setCPU(const std::string &Name) override {
+    bool isValid = isValidCPUName(Name);
+    if (isValid) this->CPU = Name;
+    return isValid;
+  }
+
 protected:
   std::string CPU;
 };
 
 } // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
 // Driver code

lib/CodeGen/CGCall.cpp

if (FD && FD->hasAttr<TargetAttr>()) {
         Features.push_back((it->second ? "+" : "-") + it->first().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.
       const auto *TD = FD->getAttr<TargetAttr>();
       TargetAttr::ParsedTargetAttr ParsedAttr = TD->parse();
-      if (ParsedAttr.second != "")
-        TargetCPU = ParsedAttr.second;
+      if (ParsedAttr.Architecture != "")
+        TargetCPU = ParsedAttr.Architecture;
       if (TargetCPU != "")
         FuncAttrs.addAttribute("target-cpu", TargetCPU);
       if (!Features.empty()) {
         std::sort(Features.begin(), Features.end());
         FuncAttrs.addAttribute(
             "target-features",
             llvm::join(Features.begin(), Features.end(), ","));
       }

lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
                                           const FunctionDecl *FD) {
   StringRef TargetCPU = Target.getTargetOpts().CPU;
   if (const auto *TD = FD->getAttr<TargetAttr>()) {
     // If we have a TargetAttr build up the feature map based on that.
     TargetAttr::ParsedTargetAttr ParsedAttr = TD->parse();
 
     // Make a copy of the features as passed on the command line into the
     // beginning of the additional features from the function to override.
-    ParsedAttr.first.insert(ParsedAttr.first.begin(),
+    ParsedAttr.Features.insert(ParsedAttr.Features.begin(),
                             Target.getTargetOpts().FeaturesAsWritten.begin(),
                             Target.getTargetOpts().FeaturesAsWritten.end());
 
-    if (ParsedAttr.second != "")
-      TargetCPU = ParsedAttr.second;
+    if (ParsedAttr.Architecture != "")
+      TargetCPU = ParsedAttr.Architecture ;
 
     // Now populate the feature map, first with the TargetCPU which is either
     // the default or a new one from the target attribute string. Then we'll use
     // the passed in features (FeaturesAsWritten) along with the new ones from
     // the attribute.
-    Target.initFeatureMap(FeatureMap, getDiags(), TargetCPU, ParsedAttr.first);
+    Target.initFeatureMap(FeatureMap, getDiags(), TargetCPU,
+                          ParsedAttr.Features);
   } else {
     Target.initFeatureMap(FeatureMap, getDiags(), TargetCPU,
                           Target.getTargetOpts().Features);
   }
 }
 
 llvm::SanitizerStatReport &CodeGenModule::getSanStats() {
   if (!SanStats)

lib/Sema/SemaDeclAttr.cpp

static void handleSectionAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   }
 
   unsigned Index = Attr.getAttributeSpellingListIndex();
   SectionAttr *NewAttr = S.mergeSectionAttr(D, Attr.getRange(), Str, Index);
   if (NewAttr)
     D->addAttr(NewAttr);
 }
 
-// Check for things we'd like to warn about, no errors or validation for now.
-// TODO: Validation should use a backend target library that specifies
-// the allowable subtarget features and cpus. We could use something like a
-// TargetCodeGenInfo hook here to do validation.
-void Sema::checkTargetAttr(SourceLocation LiteralLoc, StringRef AttrStr) {
+// 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) {
   for (auto Str : {"tune=", "fpmath="})
     if (AttrStr.find(Str) != StringRef::npos)
-      Diag(LiteralLoc, diag::warn_unsupported_target_attribute) << Str;
+      return Diag(LiteralLoc, diag::warn_unsupported_target_attribute) << Str;
+
+  TargetAttr::ParsedTargetAttr ParsedAttrs = TargetAttr::parse(AttrStr);
+  if(ParsedAttrs.DuplicateArchitecture)
+    return Diag(LiteralLoc, diag::warn_duplicate_target_attribute) << "arch";
+
+  if (!ParsedAttrs.Architecture.empty() &&
+      !Context.getTargetInfo().isValidCPUName(ParsedAttrs.Architecture))
+    return Diag(LiteralLoc, diag::warn_unsupported_target_architecture)
+           << ParsedAttrs.Architecture;
+
+  for (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)
+             << CurFeature;
+  }
+
+  return true;
 }
 
 static void handleTargetAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   StringRef Str;
   SourceLocation LiteralLoc;
-  if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str, &LiteralLoc))
+  if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str, &LiteralLoc) ||
+      !S.checkTargetAttr(LiteralLoc, Str))
     return;
-  S.checkTargetAttr(LiteralLoc, Str);
   unsigned Index = Attr.getAttributeSpellingListIndex();
   TargetAttr *NewAttr =
       ::new (S.Context) TargetAttr(Attr.getRange(), S.Context, Str, Index);
   D->addAttr(NewAttr);
 }
 
 static void handleCleanupAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   VarDecl *VD = cast<VarDecl>(D);

test/Sema/attr-target.c

 // RUN: %clang_cc1 -triple x86_64-linux-gnu  -fsyntax-only -verify %s
 
 int __attribute__((target("avx,sse4.2,arch=ivybridge"))) foo() { return 4; }
 int __attribute__((target())) bar() { return 4; } //expected-error {{'target' attribute takes one argument}}
 int __attribute__((target("tune=sandybridge"))) baz() { return 4; } //expected-warning {{Ignoring unsupported 'tune=' in the target attribute string}}
 int __attribute__((target("fpmath=387"))) walrus() { return 4; } //expected-warning {{Ignoring unsupported 'fpmath=' in the target attribute string}}
+int __attribute__((target("avx,sse4.2,arch=hiss"))) meow() {  return 4; }//expected-warning {{Ignoring unsupported architecture 'hiss' in the target attribute string}}
+int __attribute__((target("woof"))) bark() {  return 4; }//expected-warning {{Ignoring unsupported 'woof' in the target attribute string}}
+