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

[Aaron Ballman via cfe-commits]

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. ;-)

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

~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