r246610 - Migrate the target attribute parsing code into an extension off of
Eric Christopher via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 2 13:16:23 PDT 2015
On Wed, Sep 2, 2015 at 10:11 AM Aaron Ballman <aaron at aaronballman.com>
wrote:
> On Wed, Sep 2, 2015 at 1:07 PM, Eric Christopher <echristo at gmail.com>
> wrote:
> >
> >
> > On Wed, Sep 2, 2015 at 7:24 AM Aaron Ballman <aaron at 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 at 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 at lists.llvm.org
> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150902/c356dfc0/attachment-0001.html>
More information about the cfe-commits
mailing list