<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Sep 2, 2015 at 10:11 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 Wed, Sep 2, 2015 at 1:07 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:<br>
><br>
><br>
> On Wed, Sep 2, 2015 at 7:24 AM Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>> wrote:<br>
>><br>
>> 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:<br>
>> > <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>
>> > ==============================================================================<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>
><br>
> Heh. Sure. I don't really like mutable unless there's no other choice, but I<br>
> can do it here, however, it also sounds like you don't want them to be data<br>
> members at all? Did you want me to just reparse them or try to put a cache<br>
> somewhere?<br>
<br>
I'm fine with them being data members. The usual pattern is:<br>
<br>
let AdditionalMembers = [{<br>
private:<br>
<data members><br>
<br>
public:<br>
<functionality><br>
}];<br>
<br></blockquote><div><br></div><div>Yeah, there's only one case and I think Richard is going to fix it. In general, anything there is going to leak so I'm going to do this a different way and just reparse for now. If it becomes an issue (I really only plan on using this in about 3 places) then we can look into caching it on the ASTContext somewhere (or allocating it via the bump pointer allocator there).</div><div><br></div><div>I've added a comment here:</div><div><br></div><div><div>Committing to <a href="https://llvm.org/svn/llvm-project/cfe/trunk">https://llvm.org/svn/llvm-project/cfe/trunk</a> ...</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>M<span class="Apple-tab-span" style="white-space:pre"> </span>include/clang/Basic/Attr.td</div><div>Committed r246701</div></div><div><br></div><div>to basically tell people not to use this pattern :)</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">
><br>
>><br>
>> > + 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<br>
>> > 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<br>
>> > the<br>
>> > + // attributes on the function.<br>
>> > + if (Feature.startswith("fpmath=") ||<br>
>> > 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:<br>
>> > <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>
>> > ==============================================================================<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 =<br>
>> > 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<br>
>> > 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>
><br>
> Probably a bit, but I can if you'd like :)<br>
<br>
lol, we can leave it be then. ;-)<br>
<br>
~Aaron<br>
<br>
><br>
> -eric<br>
><br>
>><br>
>> ~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<br>
>> > checking<br>
>> > - // overall feature validity for the function with the rest of<br>
>> > 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<br>
>> > is either<br>
>> > // the default or a new one from the target attribute string.<br>
>> > Then we'll<br>
>> > // use the passed in features (FeaturesAsWritten) along with the<br>
>> > 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>