[PATCH] D35519: Add SEMA checking to attribute 'target'
erichkeane abandoned this revision. erichkeane added a comment. Going to split this into a few patches as Eric requested. https://reviews.llvm.org/D35519 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35519: Add SEMA checking to attribute 'target'
erichkeane added a comment. In https://reviews.llvm.org/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. https://reviews.llvm.org/D35519 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35519: Add SEMA checking to attribute 'target'
echristo added a comment.
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
Comment at: test/Sema/attr-target.c:10
+int __attribute__((target("arch="))) turtle() { return 4; } // no warning,
same as saying 'nothing'.
+int __attribute__((target("arch=hiss,arch=woof"))) pine_tree() { return 4; }
//expected-warning {{ignoring duplicate 'arch=' in the target attribute string}}
+
Precedence issue maybe, but I'd have expected this to error on the architecture
name rather than the duplicate?
https://reviews.llvm.org/D35519
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35519: Add SEMA checking to attribute 'target'
erichkeane marked 2 inline comments as done. erichkeane added a comment. 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. https://reviews.llvm.org/D35519 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35519: Add SEMA checking to attribute 'target'
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
A few small nits, but otherwise LGTM
Comment at: lib/Sema/SemaDeclAttr.cpp:2999-3000
+bool Sema::checkTargetAttr(SourceLocation LiteralLoc, StringRef AttrStr) {
+ // Error message enums. Enum vs enum class since the scope is limited,
+ // and it otherwise simplifies the error messages.
+ enum FirstParam { unsupported, duplicate};
I don't think this comment is needed (the code is pretty obvious), so I'd just
drop it entirely.
Comment at: lib/Sema/SemaDeclAttr.cpp:3001-3002
+ // and it otherwise simplifies the error messages.
+ enum FirstParam { unsupported, duplicate};
+ enum SecondParam { None, architecture };
for (auto Str : {"tune=", "fpmath="})
enumerator casing should be Unsupported, Duplicate, Architecture.
https://reviews.llvm.org/D35519
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35519: Add SEMA checking to attribute 'target'
erichkeane updated this revision to Diff 107101.
erichkeane marked 9 inline comments as done.
erichkeane added a comment.
Thanks for the feedback Aaron. This should fix everything you had an issue
with.
-Erich
https://reviews.llvm.org/D35519
Files:
include/clang/Basic/Attr.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TargetInfo.h
include/clang/Sema/Sema.h
lib/Basic/Targets.cpp
lib/CodeGen/CGCall.cpp
lib/CodeGen/CodeGenModule.cpp
lib/Sema/SemaDeclAttr.cpp
test/Sema/attr-target.c
Index: test/Sema/attr-target.c
===
--- test/Sema/attr-target.c
+++ test/Sema/attr-target.c
@@ -2,7 +2,12 @@
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("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}}
+int __attribute__((target("arch="))) turtle() { return 4; } // no warning, same as saying 'nothing'.
+int __attribute__((target("arch=hiss,arch=woof"))) pine_tree() { return 4; } //expected-warning {{ignoring duplicate 'arch=' in the target attribute string}}
+
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -2993,22 +2993,42 @@
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) {
+ // Error message enums. Enum vs enum class since the scope is limited,
+ // and it otherwise simplifies the error messages.
+ enum FirstParam { unsupported, duplicate};
+ enum SecondParam { None, architecture };
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) << unsupported << None << Str;
+
+ TargetAttr::ParsedTargetAttr ParsedAttrs = TargetAttr::parse(AttrStr);
+ if (ParsedAttrs.DuplicateArchitecture)
+return Diag(LiteralLoc, diag::warn_unsupported_target_attribute) << duplicate << None << "arch=";
+
+ if (!ParsedAttrs.Architecture.empty() &&
+ !Context.getTargetInfo().isValidCPUName(ParsedAttrs.Architecture))
+return Diag(LiteralLoc, diag::warn_unsupported_target_attribute) << unsupported << architecture
+ << ParsedAttrs.Architecture;
+
+ for (const auto : 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;
+ }
+
+ return true;
}
static void handleTargetAttr(Sema , Decl *D, const AttributeList ) {
StringRef Str;
SourceLocation LiteralLoc;
- if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str, ))
+ if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str, ) ||
+ !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);
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -4499,18 +4499,19 @@
// Make a copy of the features as passed on the command line into the
// beginning of the additional features from the function to
[PATCH] D35519: Add SEMA checking to attribute 'target'
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;
+def warn_duplicate_target_attribute
+: Warning<"Ignoring duplicate '%0' in the target attribute string">,
+InGroup;
+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;
+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 ) 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 : 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 : 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35519: Add SEMA checking to attribute 'target'
erichkeane created this revision.
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.
https://reviews.llvm.org/D35519
Files:
include/clang/Basic/Attr.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Basic/TargetInfo.h
include/clang/Sema/Sema.h
lib/Basic/Targets.cpp
lib/CodeGen/CGCall.cpp
lib/CodeGen/CodeGenModule.cpp
lib/Sema/SemaDeclAttr.cpp
test/Sema/attr-target.c
Index: test/Sema/attr-target.c
===
--- test/Sema/attr-target.c
+++ test/Sema/attr-target.c
@@ -4,5 +4,8 @@
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}}
+
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -2993,22 +2993,38 @@
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 : 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 , Decl *D, const AttributeList ) {
StringRef Str;
SourceLocation LiteralLoc;
- if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str, ))
+ if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str, ) ||
+ !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);
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -4499,18 +4499,19 @@
// 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 =
