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 10:07:58 PDT 2015
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?
> > + 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 :)
-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/f8559681/attachment.html>
More information about the cfe-commits
mailing list