r246610 - Migrate the target attribute parsing code into an extension off of

Aaron Ballman via cfe-commits cfe-commits at lists.llvm.org
Wed Sep 2 10:11:37 PDT 2015


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>
}];

>
>>
>> > +    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


More information about the cfe-commits mailing list