<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Sep 2, 2015 at 7:24 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">A few nits in addition to what Ben pointed out...<br>
<br>
On Tue, Sep 1, 2015 at 8:12 PM, Eric Christopher via cfe-commits<br>
<<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
> Author: echristo<br>
> Date: Tue Sep 1 19:12:02 2015<br>
> New Revision: 246610<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=246610&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=246610&view=rev</a><br>
> Log:<br>
> Migrate the target attribute parsing code into an extension off of<br>
> the main attribute and cache the results so we don't have to parse<br>
> a single attribute more than once.<br>
><br>
> This reapplies r246596 with a fix for an uninitialized class member,<br>
> and a couple of cleanups and formatting changes.<br>
><br>
> Modified:<br>
> cfe/trunk/include/clang/Basic/Attr.td<br>
> cfe/trunk/lib/CodeGen/CGCall.cpp<br>
><br>
> Modified: cfe/trunk/include/clang/Basic/Attr.td<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=246610&r1=246609&r2=246610&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=246610&r1=246609&r2=246610&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/Basic/Attr.td (original)<br>
> +++ cfe/trunk/include/clang/Basic/Attr.td Tue Sep 1 19:12:02 2015<br>
> @@ -1315,9 +1315,52 @@ def Pascal : InheritableAttr {<br>
><br>
> def Target : InheritableAttr {<br>
> let Spellings = [GCC<"target">];<br>
> - let Args = [StringArgument<"features">];<br>
> + let Args = [StringArgument<"featuresStr">];<br>
> let Subjects = SubjectList<[Function], ErrorDiag>;<br>
> let Documentation = [TargetDocs];<br>
> + let AdditionalMembers = [{<br>
> + StringRef CPU;<br>
> + std::vector<std::string> Features;<br>
> + bool Parsed = false;<br>
<br>
Additional members are all public, can you privatize the data members?<br>
Then you can make them mutable and fix the const-correctness<br>
regression. ;-)<br>
<br></blockquote><div><br></div><div>Heh. Sure. I don't really like mutable unless there's no other choice, but I can do it here, however, it also sounds like you don't want them to be data members at all? Did you want me to just reparse them or try to put a cache somewhere?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + StringRef getCPU() {<br>
> + if (!Parsed)<br>
> + parse();<br>
> + return CPU;<br>
> + }<br>
> + std::vector<std::string> &getFeatures() {<br>
> + if (!Parsed)<br>
> + parse();<br>
> + return Features;<br>
> + }<br>
> + void parse() {<br>
> + SmallVector<StringRef, 1> AttrFeatures;<br>
> + getFeaturesStr().split(AttrFeatures, ",");<br>
> +<br>
> + // Grab the various features and prepend a "+" to turn on the feature to<br>
> + // the backend and add them to our existing set of features.<br>
> + for (auto &Feature : AttrFeatures) {<br>
> + // Go ahead and trim whitespace rather than either erroring or<br>
> + // accepting it weirdly.<br>
> + Feature = Feature.trim();<br>
> +<br>
> + // We don't support cpu tuning this way currently.<br>
> + // TODO: Support the fpmath option. It will require checking<br>
> + // overall feature validity for the function with the rest of the<br>
> + // attributes on the function.<br>
> + if (Feature.startswith("fpmath=") || Feature.startswith("tune="))<br>
> + continue;<br>
> +<br>
> + // While we're here iterating check for a different target cpu.<br>
> + if (Feature.startswith("arch="))<br>
> + CPU = Feature.split("=").second.trim();<br>
> + else if (Feature.startswith("no-"))<br>
> + Features.push_back("-" + Feature.split("-").second.str());<br>
> + else<br>
> + Features.push_back("+" + Feature.str());<br>
> + }<br>
> + Parsed = true;<br>
> + }<br>
> + }];<br>
> }<br>
><br>
> def TransparentUnion : InheritableAttr {<br>
><br>
> Modified: cfe/trunk/lib/CodeGen/CGCall.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=246610&r1=246609&r2=246610&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=246610&r1=246609&r2=246610&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/CodeGen/CGCall.cpp (original)<br>
> +++ cfe/trunk/lib/CodeGen/CGCall.cpp Tue Sep 1 19:12:02 2015<br>
> @@ -1499,40 +1499,19 @@ void CodeGenModule::ConstructAttributeLi<br>
> const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(TargetDecl);<br>
> if (FD && FD->hasAttr<TargetAttr>()) {<br>
> llvm::StringMap<bool> FeatureMap;<br>
> - const auto *TD = FD->getAttr<TargetAttr>();<br>
> + auto *TD = FD->getAttr<TargetAttr>();<br>
><br>
> // Make a copy of the features as passed on the command line.<br>
> std::vector<std::string> FnFeatures =<br>
> getTarget().getTargetOpts().FeaturesAsWritten;<br>
><br>
> - // Grab the target attribute string.<br>
> - StringRef FeaturesStr = TD->getFeatures();<br>
> - SmallVector<StringRef, 1> AttrFeatures;<br>
> - FeaturesStr.split(AttrFeatures, ",");<br>
> + std::vector<std::string> &AttrFeatures = TD->getFeatures();<br>
> + std::copy(AttrFeatures.begin(), AttrFeatures.end(),<br>
> + std::back_inserter(FnFeatures));<br>
><br>
> - // Grab the various features and prepend a "+" to turn on the feature to<br>
> - // the backend and add them to our existing set of features.<br>
> - for (auto &Feature : AttrFeatures) {<br>
> - // Go ahead and trim whitespace rather than either erroring or<br>
> - // accepting it weirdly.<br>
> - Feature = Feature.trim();<br>
> + if (TD->getCPU() != "")<br>
> + TargetCPU = TD->getCPU();<br>
<br>
Would this be a slight improvement, or am I splitting hairs too much? ;-)<br>
<br>
if ((StringRef S = TD->getCPU()) != "")<br>
TargetCPU = S;<br>
<br></blockquote><div><br></div><div>Probably a bit, but I can if you'd like :)</div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
~Aaron<br>
<br>
><br>
> - // While we're here iterating check for a different target cpu.<br>
> - if (Feature.startswith("arch="))<br>
> - TargetCPU = Feature.split("=").second.trim();<br>
> - else if (Feature.startswith("tune="))<br>
> - // We don't support cpu tuning this way currently.<br>
> - ;<br>
> - else if (Feature.startswith("fpmath="))<br>
> - // TODO: Support the fpmath option this way. It will require checking<br>
> - // overall feature validity for the function with the rest of the<br>
> - // attributes on the function.<br>
> - ;<br>
> - else if (Feature.startswith("no-"))<br>
> - FnFeatures.push_back("-" + Feature.split("-").second.str());<br>
> - else<br>
> - FnFeatures.push_back("+" + Feature.str());<br>
> - }<br>
> // Now populate the feature map, first with the TargetCPU which is either<br>
> // the default or a new one from the target attribute string. Then we'll<br>
> // use the passed in features (FeaturesAsWritten) along with the new ones<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>