<div dir="ltr">Hi Aaron,<div><br></div><div>I've committed the patch with the quoting in r239635.</div><div><br></div><div>As we spoke on irc we're not doing individual string checking on these two substrings of possible program strings so I'm going to leave them as it is, if we do end up checking or matching then yes, I agree a StringSwitch would make sense.</div><div><br></div><div>Thanks a ton for the review :)</div><div><br></div><div>-eric</div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Jun 12, 2015 at 6:03 AM Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Thu, Jun 11, 2015 at 9:36 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:<br>
> Author: echristo<br>
> Date: Thu Jun 11 20:36:05 2015<br>
> New Revision: 239583<br>
><br>
> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D239583-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=lMHWOpuM3TdduvymzU7ygHmMf469Dft4az-_1hNHMuk&s=033gEOXmN6hl8jd7TKEGk6ZEiosTR-9lG0qlliisJ00&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=239583&view=rev</a><br>
> Log:<br>
> Add a warning for unsupported elements of the target attribute.<br>
><br>
> Since we're ignoring the tune= and fpmath= attributes go ahead<br>
> and add a warning alerting people to the fact that we're going<br>
> to ignore that part of it during code generation and tie it to<br>
> the attribute warning set.<br>
><br>
> Modified:<br>
> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
> cfe/trunk/include/clang/Sema/Sema.h<br>
> cfe/trunk/lib/Sema/SemaDeclAttr.cpp<br>
> cfe/trunk/test/Sema/attr-target.c<br>
><br>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_include_clang_Basic_DiagnosticSemaKinds.td-3Frev-3D239583-26r1-3D239582-26r2-3D239583-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=lMHWOpuM3TdduvymzU7ygHmMf469Dft4az-_1hNHMuk&s=lS3gHMb-Li2B0AFq3IVUwVSr9polADRQ0VrWq02Ih5U&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=239583&r1=239582&r2=239583&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jun 11 20:36:05 2015<br>
> @@ -1974,8 +1974,11 @@ def err_attribute_too_few_arguments : Er<br>
> def err_attribute_invalid_vector_type : Error<"invalid vector element type %0">;<br>
> def err_attribute_bad_neon_vector_size : Error<<br>
> "Neon vector size must be 64 or 128 bits">;<br>
> -def err_attribute_unsupported : Error<<br>
> - "%0 attribute is not supported for this target">;<br>
> +def warn_unsupported_target_attribute<br>
> + : Warning<"Ignoring unsupported %0 in the target attribute string">,<br>
> + InGroup<IgnoredAttributes>;<br>
<br>
Please ensure that %0 is quoted properly.<br>
<br>
> +def err_attribute_unsupported<br>
> + : Error<"%0 attribute is not supported for this target">;<br>
> // The err_*_attribute_argument_not_int are seperate because they're used by<br>
> // VerifyIntegerConstantExpression.<br>
> def err_aligned_attribute_argument_not_int : Error<<br>
><br>
> Modified: cfe/trunk/include/clang/Sema/Sema.h<br>
> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_include_clang_Sema_Sema.h-3Frev-3D239583-26r1-3D239582-26r2-3D239583-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=lMHWOpuM3TdduvymzU7ygHmMf469Dft4az-_1hNHMuk&s=aU7Hq5PwmPTyP31LgYesjGMwtQ_hdgxKX-GffNBy94Q&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=239583&r1=239582&r2=239583&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/Sema/Sema.h (original)<br>
> +++ cfe/trunk/include/clang/Sema/Sema.h Thu Jun 11 20:36:05 2015<br>
> @@ -2820,6 +2820,7 @@ public:<br>
> unsigned ArgNum, StringRef &Str,<br>
> SourceLocation *ArgLocation = nullptr);<br>
> bool checkSectionName(SourceLocation LiteralLoc, StringRef Str);<br>
> + void checkTargetAttr(SourceLocation LiteralLoc, StringRef Str);<br>
> bool checkMSInheritanceAttrOnDefinition(<br>
> CXXRecordDecl *RD, SourceRange Range, bool BestCase,<br>
> MSInheritanceAttr::Spelling SemanticSpelling);<br>
><br>
> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp<br>
> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_lib_Sema_SemaDeclAttr.cpp-3Frev-3D239583-26r1-3D239582-26r2-3D239583-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=lMHWOpuM3TdduvymzU7ygHmMf469Dft4az-_1hNHMuk&s=_vo9pDEa0WQXPeWfoq3HMXbQ-4cmHRM5k2nUJk_5yf4&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=239583&r1=239582&r2=239583&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)<br>
> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Thu Jun 11 20:36:05 2015<br>
> @@ -2397,14 +2397,22 @@ static void handleSectionAttr(Sema &S, D<br>
> D->addAttr(NewAttr);<br>
> }<br>
><br>
> +// Check for things we'd like to warn about, no errors or validation for now.<br>
> +// TODO: Validation should use a backend target library that specifies<br>
> +// the allowable subtarget features and cpus. We could use something like a<br>
> +// TargetCodeGenInfo hook here to do validation.<br>
> +void Sema::checkTargetAttr(SourceLocation LiteralLoc, StringRef AttrStr) {<br>
> + for (auto Str : {"tune=", "fpmath="})<br>
> + if (AttrStr.find(Str) != StringRef::npos)<br>
> + Diag(LiteralLoc, diag::warn_unsupported_target_attribute) << Str;<br>
<br>
I know this only has two entries right now and is going to change<br>
someday, but a StringSwitch would make me happier.<br>
<br>
> +}<br>
> +<br>
> static void handleTargetAttr(Sema &S, Decl *D, const AttributeList &Attr) {<br>
> - // TODO: Validation should use a backend target library that specifies<br>
> - // the allowable subtarget features and cpus. We could use something like a<br>
> - // TargetCodeGenInfo hook here to do validation.<br>
> StringRef Str;<br>
> SourceLocation LiteralLoc;<br>
> if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str, &LiteralLoc))<br>
> return;<br>
> + S.checkTargetAttr(LiteralLoc, Str);<br>
> unsigned Index = Attr.getAttributeSpellingListIndex();<br>
> TargetAttr *NewAttr =<br>
> ::new (S.Context) TargetAttr(Attr.getRange(), S.Context, Str, Index);<br>
><br>
> Modified: cfe/trunk/test/Sema/attr-target.c<br>
> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_cfe_trunk_test_Sema_attr-2Dtarget.c-3Frev-3D239583-26r1-3D239582-26r2-3D239583-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=lMHWOpuM3TdduvymzU7ygHmMf469Dft4az-_1hNHMuk&s=HtBzkpTuHWC8I5dLvxl1asReR0wknt_iK4zLC-lx-fk&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/attr-target.c?rev=239583&r1=239582&r2=239583&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/Sema/attr-target.c (original)<br>
> +++ cfe/trunk/test/Sema/attr-target.c Thu Jun 11 20:36:05 2015<br>
> @@ -2,5 +2,7 @@<br>
><br>
> int __attribute__((target("avx,sse4.2,arch=ivybridge"))) foo() { return 4; }<br>
> int __attribute__((target())) bar() { return 4; } //expected-error {{'target' attribute takes one argument}}<br>
> +int __attribute__((target("tune=sandybridge"))) baz() { return 4; } //expected-warning {{Ignoring unsupported tune= in the target attribute string}}<br>
> +int __attribute__((target("fpmath=387"))) walrus() { return 4; } //expected-warning {{Ignoring unsupported fpmath= in the target attribute string}}<br>
<br>
Thanks!<br>
<br>
~Aaron<br>
<br>
><br>
><br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>