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