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:42:47 PDT 2015
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 <echristo at gmail.com> wrote:
> 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/054a0d75/attachment-0001.html>
More information about the cfe-commits
mailing list