WIP Make attribute target work better with AArch64

[erichkeane]

Summary

This bug report: https://bugs.llvm.org/show_bug.cgi?id=43448

Highlights 2 issues with the attribute target AArch64 support.

The first is that the cpu check needs to check the arch rather than
the CPU.

The second is that the AArch64 implementation of GCC's target supports
+<FEATURE> for these targets.

This patch adds these two.

However, passing the AArch64 architecture names in target-cpu
isn't supported by LLVM:

('armv8-a' is not a recognized processor for this target (ignoring processor))

This likely needs to be fixed before this patch can go in.

Note that this is necessary for the ClangBuiltLinux effort.

[nickdesaulniers]

Thanks for the patch. No comment if this is "the right thing to do," just basic code review comments.

clang/include/clang/Basic/Attr.td

2107

C++ in a .td file? *mind blown*

2126

This comment is now obsoleted by this change, I think?

2154

So many definitions of parse! If this one had Features be defaulted to getFeaturesStr(), then we could eliminate the definition on 2107.

clang/lib/Basic/Targets/AArch64.cpp

112

I think it could be a minus sign, too?

-mattr=-neon,-fp64,-d32
-mattr=+fullfp16,+thumb-mode

clang/lib/Sema/SemaDeclAttr.cpp

2976

It's too bad the TargetAttr has no notion of TargetInfo; it's kind of ugly to have to pass this in.

llvm/include/llvm/ADT/StringRef.h

783–784

So for MaxSplit and KeepEmpty I feel like YAGNI may apply here. Default arguments that are never changed in the callers is a slight code smell.

[erichkeane]

clang/include/clang/Basic/Attr.td

2154

This one is static.

clang/lib/Basic/Targets/AArch64.cpp

112

attribute-target doesn't support 'minus'. It is additive only.

clang/lib/Sema/SemaDeclAttr.cpp

2976

Typically we don't have attributes be context/targetinfo aware. It is a little sucky to have to pass this in, but I didn't think it too evil.

llvm/include/llvm/ADT/StringRef.h

783–784

Thats perhaps true. I was going for consistency with "split", since that is what I copied it from :)

[labrinea]

However, passing the AArch64 architecture names in target-cpu isn't supported by LLVM

The Clang documentation suggests that arch is used to override the CPU, not the Architecture (which is rather confusing if you ask me). GCC makes more sense having separate target attributes for CPU and Architecture (see the equivalent GCC documentation). I think target-cpu should remain generic when it is not explicitly specified either on the command line (-mcpu) or as a function attribute (i.e target("arch=cortex-a57")). However, if the function attribute specifies an Architecture (i.e target("arch=armv8.4a")), I agree we should favor the subtarget features corresponding to armv8.4 over those of the command line. Similarly we should favor the subtarget features corresponding to cortex-a57 (not sure if we do so atm - I think we don't). ARM and AArch64 have a way to list the implied target features using the TargetParser but we can't directly use that in CodeGenModule because it's tied to the backend.

[erichkeane]

Ah, Ugg... GCC must implement the target parsing handling different on a per-architecture basis (ARM vs X86). We've only got parsing in place to do the x86 way. That ends up making this a much more difficult thing to maintain I suspect.

I also don't know if we have much of a way of expressing this in IR at the moment. @echristo : How much do you know of this?

[labrinea]

ARM and AArch64 have a way to list the implied target features using the TargetParser but we can't directly use that in CodeGenModule because it's tied to the backend.

It seems that initFeatureMap allows this, but it's currently limited to inferring features from the CPU name. The ARMTargetInfo class provides an override of this function. I think for this patch to work we would need an override for AArch64 with the ability to handle Arch names.

[fhahn]

Thanks for putting up the patch! Some comments inline.

clang/lib/Basic/Targets/AArch64.cpp

108

This does not seem right. The AArch64 target makes the distinction between CPU and architecture names. Unfortunately the documentation for parseCPUArch is non-existent, but it checks if the passed in CPU name matches a valid CPU name and returns the corresponding architecture of the CPU.

. I think with the change, isValidCPUName("cortex-a57") will return false, whereas it should return true. isValidCPUName("armv8-a") will return true instead of false.

I think it would be better to use parseArch directly to validate the "arch=" attribute (or add an isValidArchName here).

Also, CPU features/extensions can be validate with llvm::AArch64::parseArchExt().

clang/test/CodeGen/aarch64-target-attr.c

1

Can we also add a few negative tests, including. __attribute__((target("arch=armv8.4-a-lse"))), an invalid architecture name and an invalid CPU feature?

[fhahn]

Hi Eric,

Thanks for putting up the initial patch. Are you interested in pushing it forward? Or would you prefer someone else taking over?

Cheers,
Florian

[erichkeane]

In D68050#1763191, @fhahn wrote:

Hi Eric,

Thanks for putting up the initial patch. Are you interested in pushing it forward? Or would you prefer someone else taking over?

Cheers,
Florian

You're welcome to take it over. The details of arch vs CPU on Arm are a bit foreign to me, and I didn't come up with a good way to solve this cleanly. I'm not sure I'll get bandwidth to do anything with this anytime soon as well, but I'll definitely review anything you have :)

clang/include/clang/Basic/Attr.td

struct ParsedTargetAttr {
       std::vector<std::string> Features;
       StringRef Architecture;
       bool DuplicateArchitecture = false;
       bool operator ==(const ParsedTargetAttr &Other) const {
         return DuplicateArchitecture == Other.DuplicateArchitecture &&
                Architecture == Other.Architecture && Features == Other.Features;
       }
     };
-    ParsedTargetAttr parse() const {
-      return parse(getFeaturesStr());
+    ParsedTargetAttr parse(StringRef SplitChars) const {
+      return parse(getFeaturesStr(), SplitChars);
     }
 
-    StringRef getArchitecture() const {
+    StringRef getArchitecture(StringRef SplitChars) const {
       StringRef Features = getFeaturesStr();
       if (Features == "default") return {};
 
       SmallVector<StringRef, 1> AttrFeatures;
-      Features.split(AttrFeatures, ",");
+      Features.split_on_one_of(AttrFeatures, SplitChars);
 
       for (auto &Feature : AttrFeatures) {
         Feature = Feature.trim();
         if (Feature.startswith("arch="))
           return Feature.drop_front(sizeof("arch=") - 1);
       }
       return "";
     }
 
     // Gets the list of features as simple string-refs with no +/- or 'no-'.
     // Only adds the items to 'Out' that are additions.
-    void getAddedFeatures(llvm::SmallVectorImpl<StringRef> &Out) const {
+    void getAddedFeatures(StringRef SplitChars,
+                          llvm::SmallVectorImpl<StringRef> &Out) const {
       StringRef Features = getFeaturesStr();
       if (Features == "default") return;
 
       SmallVector<StringRef, 1> AttrFeatures;
-      Features.split(AttrFeatures, ",");
+      Features.split_on_one_of(AttrFeatures, SplitChars);
 
       for (auto &Feature : AttrFeatures) {
         Feature = Feature.trim();
 
         if (!Feature.startswith("no-") && !Feature.startswith("arch=") &&
             !Feature.startswith("fpmath=") && !Feature.startswith("tune="))
           Out.push_back(Feature);
       }
     }
 
     template<class Compare>
-    ParsedTargetAttr parse(Compare cmp) const {
-      ParsedTargetAttr Attrs = parse();
+    ParsedTargetAttr parse(Compare cmp, StringRef SplitChars) const {
+      ParsedTargetAttr Attrs = parse(SplitChars);
       llvm::sort(std::begin(Attrs.Features), std::end(Attrs.Features), cmp);
       return Attrs;
     }
 
     bool isDefaultVersion() const { return getFeaturesStr() == "default"; }
 
-    static ParsedTargetAttr parse(StringRef Features) {
+    static ParsedTargetAttr parse(StringRef Features, StringRef SplitChars) {
       ParsedTargetAttr Ret;
       if (Features == "default") return Ret;
       SmallVector<StringRef, 1> AttrFeatures;
-      Features.split(AttrFeatures, ",");
+      Features.split_on_one_of(AttrFeatures, SplitChars);
 
       // 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();
 

clang/include/clang/Basic/TargetInfo.h

public:
   /// Fill a SmallVectorImpl with the valid values to setCPU.
   virtual void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const {}
 
   /// brief Determine whether this TargetInfo supports the given CPU name.
   virtual bool isValidCPUName(StringRef Name) const {
     return true;
   }
 
+  /// Whether the '+' character as a feature separator is valid for the
+  /// Attribute-Target handling. AArch64 in GCC does, so we likely should as
+  /// well.
+  virtual StringRef getTargetAttrSplitChars() const {
+    return ",";
+  }
+
   /// Use the specified ABI.
   ///
   /// \return False on error (invalid ABI name).
   virtual bool setABI(const std::string &Name) {
     return false;
   }
 
   /// Use the specified unit for FP math.

clang/lib/Basic/Targets/AArch64.h

 
 public:
   AArch64TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts);
 
   StringRef getABI() const override;
   bool setABI(const std::string &Name) override;
 
   bool isValidCPUName(StringRef Name) const override;
+  StringRef getTargetAttrSplitChars() const override;
   void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const override;
   bool setCPU(const std::string &Name) override;
 
   bool useFP16ConversionIntrinsics() const override {
     return false;
   }
 
   void getTargetDefinesARMV81A(const LangOptions &Opts,

clang/lib/Basic/Targets/AArch64.cpp

if (Name != "aapcs" && Name != "darwinpcs")
     return false;
 
   ABI = Name;
   return true;
 }
 
 bool AArch64TargetInfo::isValidCPUName(StringRef Name) const {
   return Name == "generic" ||
-         llvm::AArch64::parseCPUArch(Name) != llvm::AArch64::ArchKind::INVALID;
+         llvm::AArch64::parseArch(Name) != llvm::AArch64::ArchKind::INVALID;
+}
+
+StringRef AArch64TargetInfo::getTargetAttrSplitChars() const {
+  return ",+";
 }
 
 bool AArch64TargetInfo::setCPU(const std::string &Name) {
   return isValidCPUName(Name);
 }
 
 void AArch64TargetInfo::fillValidCPUList(
     SmallVectorImpl<StringRef> &Values) const {

clang/lib/CodeGen/CodeGenModule.cpp

 
 static void AppendTargetMangling(const CodeGenModule &CGM,
                                  const TargetAttr *Attr, raw_ostream &Out) {
   if (Attr->isDefaultVersion())
     return;
 
   Out << '.';
   const TargetInfo &Target = CGM.getTarget();
-  TargetAttr::ParsedTargetAttr Info =
-      Attr->parse([&Target](StringRef LHS, StringRef RHS) {
+  TargetAttr::ParsedTargetAttr Info = Attr->parse(
+      [&Target](StringRef LHS, StringRef RHS) {
         // Multiversioning doesn't allow "no-${feature}", so we can
         // only have "+" prefixes here.
         assert(LHS.startswith("+") && RHS.startswith("+") &&
                "Features should always have a prefix.");
         return Target.multiVersionSortPriority(LHS.substr(1)) >
                Target.multiVersionSortPriority(RHS.substr(1));
-      });
+      },
+      Target.getTargetAttrSplitChars());
 
   bool IsFirst = true;
 
   if (!Info.Architecture.empty()) {
     IsFirst = false;
     Out << "arch_" << Info.Architecture;
   }
 
if (TD || SD) {
     for (const llvm::StringMap<bool>::value_type &Entry : FeatureMap)
       Features.push_back((Entry.getValue() ? "+" : "-") + Entry.getKey().str());
 
     // Now add the target-cpu and target-features to the function.
     // While we populated the feature map above, we still need to
     // get and parse the target attribute so we can get the cpu for
     // the function.
     if (TD) {
-      TargetAttr::ParsedTargetAttr ParsedAttr = TD->parse();
+      TargetAttr::ParsedTargetAttr ParsedAttr =
+          TD->parse(getTarget().getTargetAttrSplitChars());
       if (ParsedAttr.Architecture != "" &&
           getTarget().isValidCPUName(ParsedAttr.Architecture))
         TargetCPU = ParsedAttr.Architecture;
     }
   } else {
     // Otherwise just add the existing target cpu and target features to the
     // function.
     Features = getTarget().getTargetOpts().Features;
getContext().forEachMultiversionedFunctionVersion(
               Func = GetAddrOfFunction(CurGD, Ty, /*ForVTable=*/false,
                                        /*DontDefer=*/false, ForDefinition);
             }
             assert(Func && "This should have just been created");
           }
 
           const auto *TA = CurFD->getAttr<TargetAttr>();
           llvm::SmallVector<StringRef, 8> Feats;
-          TA->getAddedFeatures(Feats);
+          TA->getAddedFeatures(getTarget().getTargetAttrSplitChars(), Feats);
 
           Options.emplace_back(cast<llvm::Function>(Func),
-                               TA->getArchitecture(), Feats);
+                               TA->getArchitecture(getTarget().getTargetAttrSplitChars()), Feats);
         });
 
     llvm::Function *ResolverFunc;
     const TargetInfo &TI = getTarget();
 
     if (TI.supportsIFunc() || FD->isTargetMultiVersion()) {
       ResolverFunc = cast<llvm::Function>(
           GetGlobalValue((getMangledName(GD) + ".resolver").str()));
void CodeGenModule::AddVTableTypeMetadata(llvm::GlobalVariable *VTable,
   if (NeedAllVtablesTypeId()) {
     llvm::Metadata *MD = llvm::MDString::get(getLLVMContext(), "all-vtables");
     VTable->addTypeMetadata(Offset.getQuantity(), MD);
   }
 }
 
 TargetAttr::ParsedTargetAttr CodeGenModule::filterFunctionTargetAttrs(const TargetAttr *TD) {
   assert(TD != nullptr);
-  TargetAttr::ParsedTargetAttr ParsedAttr = TD->parse();
+  TargetAttr::ParsedTargetAttr ParsedAttr =
+      TD->parse(getTarget().getTargetAttrSplitChars());
 
   ParsedAttr.Features.erase(
       llvm::remove_if(ParsedAttr.Features,
                       [&](const std::string &Feat) {
                         return !Target.isValidFeatureName(
                             StringRef{Feat}.substr(1));
                       }),
       ParsedAttr.Features.end());

clang/lib/Sema/SemaDecl.cpp

 
 /// Check the target attribute of the function for MultiVersion
 /// validity.
 ///
 /// Returns true if there was an error, false otherwise.
 static bool CheckMultiVersionValue(Sema &S, const FunctionDecl *FD) {
   const auto *TA = FD->getAttr<TargetAttr>();
   assert(TA && "MultiVersion Candidate requires a target attribute");
-  TargetAttr::ParsedTargetAttr ParseInfo = TA->parse();
   const TargetInfo &TargetInfo = S.Context.getTargetInfo();
+  TargetAttr::ParsedTargetAttr ParseInfo =
+      TA->parse(TargetInfo.getTargetAttrSplitChars());
   enum ErrType { Feature = 0, Architecture = 1 };
 
   if (!ParseInfo.Architecture.empty() &&
       !TargetInfo.validateCpuIs(ParseInfo.Architecture)) {
     S.Diag(FD->getLocation(), diag::err_bad_multiversion_option)
         << Architecture << ParseInfo.Architecture;
     return true;
   }
static bool PreviousDeclsHaveMultiVersionAttribute(const FunctionDecl *FD) {
   return false;
 }
 
 static bool CheckTargetCausesMultiVersioning(
     Sema &S, FunctionDecl *OldFD, FunctionDecl *NewFD, const TargetAttr *NewTA,
     bool &Redeclaration, NamedDecl *&OldDecl, bool &MergeTypeWithPrevious,
     LookupResult &Previous) {
   const auto *OldTA = OldFD->getAttr<TargetAttr>();
-  TargetAttr::ParsedTargetAttr NewParsed = NewTA->parse();
+  TargetAttr::ParsedTargetAttr NewParsed =
+      NewTA->parse(S.getASTContext().getTargetInfo().getTargetAttrSplitChars());
   // Sort order doesn't matter, it just needs to be consistent.
   llvm::sort(NewParsed.Features);
 
   // If the old decl is NOT MultiVersioned yet, and we don't cause that
   // to change, this is a simple redeclaration.
   if (!NewTA->isDefaultVersion() &&
       (!OldTA || OldTA->getFeaturesStr() == NewTA->getFeaturesStr()))
     return false;
static bool CheckTargetCausesMultiVersioning(
 
   if (CheckMultiVersionValue(S, OldFD)) {
     S.Diag(NewFD->getLocation(), diag::note_multiversioning_caused_here);
     NewFD->setInvalidDecl();
     return true;
   }
 
   TargetAttr::ParsedTargetAttr OldParsed =
-      OldTA->parse(std::less<std::string>());
+      OldTA->parse(std::less<std::string>(),
+                   S.getASTContext().getTargetInfo().getTargetAttrSplitChars());
 
   if (OldParsed == NewParsed) {
     S.Diag(NewFD->getLocation(), diag::err_multiversion_duplicate);
     S.Diag(OldFD->getLocation(), diag::note_previous_declaration);
     NewFD->setInvalidDecl();
     return true;
   }
 
if ((OldMVType == MultiVersionKind::Target &&
     S.Diag(NewFD->getLocation(), diag::err_multiversion_types_mixed);
     S.Diag(OldFD->getLocation(), diag::note_previous_declaration);
     NewFD->setInvalidDecl();
     return true;
   }
 
   TargetAttr::ParsedTargetAttr NewParsed;
   if (NewTA) {
-    NewParsed = NewTA->parse();
+    NewParsed = NewTA->parse(
+        S.getASTContext().getTargetInfo().getTargetAttrSplitChars());
     llvm::sort(NewParsed.Features);
   }
 
   bool UseMemberUsingDeclRules =
       S.CurContext->isRecord() && !NewFD->getFriendObjectKind();
 
   // Next, check ALL non-overloads to see if this is a redeclaration of a
   // previous member of the MultiVersion set.
   for (NamedDecl *ND : Previous) {
     FunctionDecl *CurFD = ND->getAsFunction();
     if (!CurFD)
       continue;
     if (S.IsOverload(NewFD, CurFD, UseMemberUsingDeclRules))
       continue;
 
     if (NewMVType == MultiVersionKind::Target) {
       const auto *CurTA = CurFD->getAttr<TargetAttr>();
       if (CurTA->getFeaturesStr() == NewTA->getFeaturesStr()) {
         NewFD->setIsMultiVersion();
         Redeclaration = true;
         OldDecl = ND;
         return false;
       }
 
-      TargetAttr::ParsedTargetAttr CurParsed =
-          CurTA->parse(std::less<std::string>());
+      TargetAttr::ParsedTargetAttr CurParsed = CurTA->parse(
+          std::less<std::string>(),
+          S.getASTContext().getTargetInfo().getTargetAttrSplitChars());
       if (CurParsed == NewParsed) {
         S.Diag(NewFD->getLocation(), diag::err_multiversion_duplicate);
         S.Diag(CurFD->getLocation(), diag::note_previous_declaration);
         NewFD->setInvalidDecl();
         return true;
       }
     } else {
       const auto *CurCPUSpec = CurFD->getAttr<CPUSpecificAttr>();

clang/lib/Sema/SemaDeclAttr.cpp

 bool Sema::checkTargetAttr(SourceLocation LiteralLoc, StringRef AttrStr) {
   enum FirstParam { Unsupported, Duplicate };
   enum SecondParam { None, Architecture };
   for (auto Str : {"tune=", "fpmath="})
     if (AttrStr.find(Str) != StringRef::npos)
       return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
              << Unsupported << None << Str;
 
-  TargetAttr::ParsedTargetAttr ParsedAttrs = TargetAttr::parse(AttrStr);
+  TargetAttr::ParsedTargetAttr ParsedAttrs = TargetAttr::parse(
+      AttrStr, Context.getTargetInfo().getTargetAttrSplitChars());
 
   if (!ParsedAttrs.Architecture.empty() &&
       !Context.getTargetInfo().isValidCPUName(ParsedAttrs.Architecture))
     return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)
            << Unsupported << Architecture << ParsedAttrs.Architecture;
 
   if (ParsedAttrs.DuplicateArchitecture)
     return Diag(LiteralLoc, diag::warn_unsupported_target_attribute)

clang/test/CodeGen/aarch64-target-attr.c

+// RUN: %clang_cc1 -triple aarch64-linux-gnu -emit-llvm -o - %s | FileCheck %s
+__attribute__((target("arch=armv8-a")))
+void f1(void){}
+//CHECK: define void @f1() [[F1_ATTRS:#[0-9]+]]
+
+__attribute__((target("arch=armv8-a+lse")))
+void f2(void){}
+//CHECK: define void @f2() [[F2_ATTRS:#[0-9]+]]
+
+__attribute__((target("arch=armv8.4-a")))
+void f3(void){}
+//CHECK: define void @f3() [[F3_ATTRS:#[0-9]+]]
+
+__attribute__((target("lse")))
+void f4(void){}
+//CHECK: define void @f4() [[F4_ATTRS:#[0-9]+]]
+
+// CHECK: [[F1_ATTRS]] = { {{.*}} "target-cpu"="armv8-a"
+// CHECK: [[F2_ATTRS]] = { {{.*}} "target-cpu"="armv8-a"{{.*}} "target-features"="+lse"
+// CHECK: [[F3_ATTRS]] = { {{.*}} "target-cpu"="armv8.4-a"
+// CHECK: [[F4_ATTRS]] = { {{.*}} "target-features"="+lse"

llvm/include/llvm/ADT/StringRef.h

public:
     /// \param A - Where to put the substrings.
     /// \param Separator - The string to split on.
     /// \param MaxSplit - The maximum number of times the string is split.
     /// \param KeepEmpty - True if empty substring should be added.
     void split(SmallVectorImpl<StringRef> &A,
                StringRef Separator, int MaxSplit = -1,
                bool KeepEmpty = true) const;
 
+    /// Split into substrings around the occurrences of a set of separators.
+    ///
+    /// Each substring is stored in \p A. If \p MaxSplit is >= 0, at most
+    /// \p MaxSplit splits are done and consequently <= \p MaxSplit + 1
+    /// elements are added to A.
+    /// If \p KeepEmpty is false, empty strings are not added to \p A. They
+    /// still count when considering \p MaxSplit
+    /// An useful invariant is that
+    /// Separator.join(A) == *this if MaxSplit == -1 and KeepEmpty == true
+    ///
+    /// \param A - Where to put the substrings.
+    /// \param Separators - The characters to split on.
+    /// \param MaxSplit - The maximum number of times the string is split.
+    /// \param KeepEmpty - True if empty substring should be added.
+    void split_on_one_of(SmallVectorImpl<StringRef> &A,
+               StringRef Separators, int MaxSplit = -1,
+               bool KeepEmpty = true) const;
+
     /// Split into substrings around the occurrences of a separator character.
     ///
     /// Each substring is stored in \p A. If \p MaxSplit is >= 0, at most
     /// \p MaxSplit splits are done and consequently <= \p MaxSplit + 1
     /// elements are added to A.
     /// If \p KeepEmpty is false, empty strings are not added to \p A. They
     /// still count when considering \p MaxSplit
     /// An useful invariant is that

llvm/lib/Support/StringRef.cpp

while (MaxSplit-- != 0) {
     S = S.slice(Idx + Separator.size(), npos);
   }
 
   // Push the tail.
   if (KeepEmpty || !S.empty())
     A.push_back(S);
 }
 
+void StringRef::split_on_one_of(SmallVectorImpl<StringRef> &A,
+                                StringRef Separators, int MaxSplit,
+                                bool KeepEmpty) const {
+  StringRef S = *this;
+
+  // Count down from MaxSplit. When MaxSplit is -1, this will just split
+  // "forever". This doesn't support splitting more than 2^31 times
+  // intentionally; if we ever want that we can make MaxSplit a 64-bit integer
+  // but that seems unlikely to be useful.
+  while (MaxSplit-- != 0) {
+    size_t Idx = S.find_first_of(Separators);
+    if (Idx == npos)
+      break;
+
+    // Push this split.
+    if (KeepEmpty || Idx > 0)
+      A.push_back(S.slice(0, Idx));
+
+    // Jump forward.
+    S = S.slice(Idx + 1, npos);
+  }
+
+  // Push the tail.
+  if (KeepEmpty || !S.empty())
+    A.push_back(S);
+}
+
 void StringRef::split(SmallVectorImpl<StringRef> &A, char Separator,
                       int MaxSplit, bool KeepEmpty) const {
   StringRef S = *this;
 
   // Count down from MaxSplit. When MaxSplit is -1, this will just split
   // "forever". This doesn't support splitting more than 2^31 times
   // intentionally; if we ever want that we can make MaxSplit a 64-bit integer
   // but that seems unlikely to be useful.