And I think I've fixed the other bits in dzur:~/sources/llvm/tools/clang> git svn dcommit Committing to https://llvm.org/svn/llvm-project/cfe/trunk ... M include/clang/Basic/Attr.td M lib/CodeGen/CGCall.cpp Committed r246706
We could eventually cache the parsing etc, but I don't think it's hugely necessary. What would end up being more helpful is to cache the attribute and then cache by attribute parsed if that makes any sense. -eric On Wed, Sep 2, 2015 at 1:16 PM Eric Christopher <echri...@gmail.com> wrote: > On Wed, Sep 2, 2015 at 10:11 AM Aaron Ballman <aa...@aaronballman.com> > wrote: > >> On Wed, Sep 2, 2015 at 1:07 PM, Eric Christopher <echri...@gmail.com> >> wrote: >> > >> > >> > On Wed, Sep 2, 2015 at 7:24 AM Aaron Ballman <aa...@aaronballman.com> >> wrote: >> >> >> >> A few nits in addition to what Ben pointed out... >> >> >> >> On Tue, Sep 1, 2015 at 8:12 PM, Eric Christopher via cfe-commits >> >> <cfe-commits@lists.llvm.org> wrote: >> >> > Author: echristo >> >> > Date: Tue Sep 1 19:12:02 2015 >> >> > New Revision: 246610 >> >> > >> >> > URL: http://llvm.org/viewvc/llvm-project?rev=246610&view=rev >> >> > Log: >> >> > Migrate the target attribute parsing code into an extension off of >> >> > the main attribute and cache the results so we don't have to parse >> >> > a single attribute more than once. >> >> > >> >> > This reapplies r246596 with a fix for an uninitialized class member, >> >> > and a couple of cleanups and formatting changes. >> >> > >> >> > Modified: >> >> > cfe/trunk/include/clang/Basic/Attr.td >> >> > cfe/trunk/lib/CodeGen/CGCall.cpp >> >> > >> >> > Modified: cfe/trunk/include/clang/Basic/Attr.td >> >> > URL: >> >> > >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=246610&r1=246609&r2=246610&view=diff >> >> > >> >> > >> ============================================================================== >> >> > --- cfe/trunk/include/clang/Basic/Attr.td (original) >> >> > +++ cfe/trunk/include/clang/Basic/Attr.td Tue Sep 1 19:12:02 2015 >> >> > @@ -1315,9 +1315,52 @@ def Pascal : InheritableAttr { >> >> > >> >> > def Target : InheritableAttr { >> >> > let Spellings = [GCC<"target">]; >> >> > - let Args = [StringArgument<"features">]; >> >> > + let Args = [StringArgument<"featuresStr">]; >> >> > let Subjects = SubjectList<[Function], ErrorDiag>; >> >> > let Documentation = [TargetDocs]; >> >> > + let AdditionalMembers = [{ >> >> > + StringRef CPU; >> >> > + std::vector<std::string> Features; >> >> > + bool Parsed = false; >> >> >> >> Additional members are all public, can you privatize the data members? >> >> Then you can make them mutable and fix the const-correctness >> >> regression. ;-) >> >> >> > >> > 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? >> >> I'm fine with them being data members. The usual pattern is: >> >> let AdditionalMembers = [{ >> private: >> <data members> >> >> public: >> <functionality> >> }]; >> >> > 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). > > I've added a comment here: > > Committing to https://llvm.org/svn/llvm-project/cfe/trunk ... > M include/clang/Basic/Attr.td > Committed r246701 > > to basically tell people not to use this pattern :) > > -eric > > >> > >> >> >> >> > + StringRef getCPU() { >> >> > + if (!Parsed) >> >> > + parse(); >> >> > + return CPU; >> >> > + } >> >> > + std::vector<std::string> &getFeatures() { >> >> > + if (!Parsed) >> >> > + parse(); >> >> > + return Features; >> >> > + } >> >> > + void parse() { >> >> > + SmallVector<StringRef, 1> AttrFeatures; >> >> > + getFeaturesStr().split(AttrFeatures, ","); >> >> > + >> >> > + // Grab the various features and prepend a "+" to turn on the >> >> > feature to >> >> > + // the backend and add them to our existing set of features. >> >> > + for (auto &Feature : AttrFeatures) { >> >> > + // Go ahead and trim whitespace rather than either erroring >> or >> >> > + // accepting it weirdly. >> >> > + Feature = Feature.trim(); >> >> > + >> >> > + // We don't support cpu tuning this way currently. >> >> > + // TODO: Support the fpmath option. It will require checking >> >> > + // overall feature validity for the function with the rest >> of >> >> > the >> >> > + // attributes on the function. >> >> > + if (Feature.startswith("fpmath=") || >> >> > Feature.startswith("tune=")) >> >> > + continue; >> >> > + >> >> > + // While we're here iterating check for a different target >> cpu. >> >> > + if (Feature.startswith("arch=")) >> >> > + CPU = Feature.split("=").second.trim(); >> >> > + else if (Feature.startswith("no-")) >> >> > + Features.push_back("-" + Feature.split("-").second.str()); >> >> > + else >> >> > + Features.push_back("+" + Feature.str()); >> >> > + } >> >> > + Parsed = true; >> >> > + } >> >> > + }]; >> >> > } >> >> > >> >> > def TransparentUnion : InheritableAttr { >> >> > >> >> > Modified: cfe/trunk/lib/CodeGen/CGCall.cpp >> >> > URL: >> >> > >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=246610&r1=246609&r2=246610&view=diff >> >> > >> >> > >> ============================================================================== >> >> > --- cfe/trunk/lib/CodeGen/CGCall.cpp (original) >> >> > +++ cfe/trunk/lib/CodeGen/CGCall.cpp Tue Sep 1 19:12:02 2015 >> >> > @@ -1499,40 +1499,19 @@ void CodeGenModule::ConstructAttributeLi >> >> > const FunctionDecl *FD = >> >> > dyn_cast_or_null<FunctionDecl>(TargetDecl); >> >> > if (FD && FD->hasAttr<TargetAttr>()) { >> >> > llvm::StringMap<bool> FeatureMap; >> >> > - const auto *TD = FD->getAttr<TargetAttr>(); >> >> > + auto *TD = FD->getAttr<TargetAttr>(); >> >> > >> >> > // Make a copy of the features as passed on the command line. >> >> > std::vector<std::string> FnFeatures = >> >> > getTarget().getTargetOpts().FeaturesAsWritten; >> >> > >> >> > - // Grab the target attribute string. >> >> > - StringRef FeaturesStr = TD->getFeatures(); >> >> > - SmallVector<StringRef, 1> AttrFeatures; >> >> > - FeaturesStr.split(AttrFeatures, ","); >> >> > + std::vector<std::string> &AttrFeatures = TD->getFeatures(); >> >> > + std::copy(AttrFeatures.begin(), AttrFeatures.end(), >> >> > + std::back_inserter(FnFeatures)); >> >> > >> >> > - // Grab the various features and prepend a "+" to turn on the >> >> > feature to >> >> > - // the backend and add them to our existing set of features. >> >> > - for (auto &Feature : AttrFeatures) { >> >> > - // Go ahead and trim whitespace rather than either erroring >> or >> >> > - // accepting it weirdly. >> >> > - Feature = Feature.trim(); >> >> > + if (TD->getCPU() != "") >> >> > + TargetCPU = TD->getCPU(); >> >> >> >> Would this be a slight improvement, or am I splitting hairs too much? >> ;-) >> >> >> >> if ((StringRef S = TD->getCPU()) != "") >> >> TargetCPU = S; >> >> >> > >> > Probably a bit, but I can if you'd like :) >> >> lol, we can leave it be then. ;-) >> >> ~Aaron >> >> > >> > -eric >> > >> >> >> >> ~Aaron >> >> >> >> > >> >> > - // While we're here iterating check for a different target >> cpu. >> >> > - if (Feature.startswith("arch=")) >> >> > - TargetCPU = Feature.split("=").second.trim(); >> >> > - else if (Feature.startswith("tune=")) >> >> > - // We don't support cpu tuning this way currently. >> >> > - ; >> >> > - else if (Feature.startswith("fpmath=")) >> >> > - // TODO: Support the fpmath option this way. It will >> require >> >> > checking >> >> > - // overall feature validity for the function with the >> rest of >> >> > the >> >> > - // attributes on the function. >> >> > - ; >> >> > - else if (Feature.startswith("no-")) >> >> > - FnFeatures.push_back("-" + >> Feature.split("-").second.str()); >> >> > - else >> >> > - FnFeatures.push_back("+" + Feature.str()); >> >> > - } >> >> > // Now populate the feature map, first with the TargetCPU >> which >> >> > is either >> >> > // the default or a new one from the target attribute string. >> >> > Then we'll >> >> > // use the passed in features (FeaturesAsWritten) along with >> the >> >> > new ones >> >> > >> >> > >> >> > _______________________________________________ >> >> > cfe-commits mailing list >> >> > cfe-commits@lists.llvm.org >> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
