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
