[PATCH] D35519: Add SEMA checking to attribute 'target'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2389-2397
 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
----------------
All of these (including the one you didn't have to touch) should be using "ignoring" instead of "Ignoring".


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2395
+    InGroup<IgnoredAttributes>;
+def warn_unsupported_target_architecture
+    : Warning<"Ignoring unsupported architecture '%0' in the target attribute "
----------------
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.


================
Comment at: lib/Basic/Targets.cpp:1015-1018
+  // 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.
----------------
I think this comment should be hoisted up to `isValidCPUName()`.


================
Comment at: lib/Basic/Targets.cpp:6416
+  bool setCPU(const std::string &Name) override {
+    return isValidCPUName(Name);
+  }
----------------
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?


================
Comment at: lib/Basic/Targets.cpp:9617
+    bool isValid = isValidCPUName(Name);
+    if (isValid) this->CPU = Name;
+    return isValid;
----------------
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).


================
Comment at: lib/Sema/SemaDeclAttr.cpp:3004
+  TargetAttr::ParsedTargetAttr ParsedAttrs = TargetAttr::parse(AttrStr);
+  if(ParsedAttrs.DuplicateArchitecture)
+    return Diag(LiteralLoc, diag::warn_duplicate_target_attribute) << "arch";
----------------
Missing a space after the `if`.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:3005
+  if(ParsedAttrs.DuplicateArchitecture)
+    return Diag(LiteralLoc, diag::warn_duplicate_target_attribute) << "arch";
+
----------------
Should this be `"arch="` to be consistent with the previous diagnostic on line 3001?


================
Comment at: lib/Sema/SemaDeclAttr.cpp:3007
+
+  if (!ParsedAttrs.Architecture.empty() &&
+      !Context.getTargetInfo().isValidCPUName(ParsedAttrs.Architecture))
----------------
Is the attribute valid when the architecture is empty?


================
Comment at: lib/Sema/SemaDeclAttr.cpp:3012
+
+  for (auto &Feature : ParsedAttrs.Features) {
+    auto CurFeature = StringRef(Feature).drop_front();// remove + or -.
----------------
Can this be made `const auto &`?


================
Comment at: lib/Sema/SemaDeclAttr.cpp:3013
+  for (auto &Feature : ParsedAttrs.Features) {
+    auto CurFeature = StringRef(Feature).drop_front();// remove + or -.
+    if (!Context.getTargetInfo().isValidFeatureName(CurFeature))
----------------
Insert a space before the comment.


================
Comment at: test/Sema/attr-target.c:9
+int __attribute__((target("woof"))) bark() {  return 4; }//expected-warning {{Ignoring unsupported 'woof' in the target attribute string}}
+
 
----------------
Please add a test case where there's a duplicate arch and an empty arch.


https://reviews.llvm.org/D35519