Store OptTable::Info::Name as a StringRef

[serge-sans-paille]

Summary

This avoids implicit conversion to StringRef at several points, which in
turns avoid redundant calls to strlen.

It also eventually gives a consistent, humble speedup in compilation
time

https://llvm-compile-time-tracker.com/compare.php?from=5f5b942823474e98e43a27d515a87ce140396c53&to=60e13b778119fc32d50dc38ff1a564a87146e9c6&stat=instructions:u

[nikic]

Build failure in pre-merge checks:

FAILED: lib/Option/CMakeFiles/LLVMOption.dir/OptTable.cpp.o
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/clang++ -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/var/lib/buildkite-agent/builds/llvm-project/build/lib/Option -I/var/lib/buildkite-agent/builds/llvm-project/llvm/lib/Option -I/var/lib/buildkite-agent/builds/llvm-project/build/include -I/var/lib/buildkite-agent/builds/llvm-project/llvm/include -gmlt -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT lib/Option/CMakeFiles/LLVMOption.dir/OptTable.cpp.o -MF lib/Option/CMakeFiles/LLVMOption.dir/OptTable.cpp.o.d -o lib/Option/CMakeFiles/LLVMOption.dir/OptTable.cpp.o -c /var/lib/buildkite-agent/builds/llvm-project/llvm/lib/Option/OptTable.cpp
/var/lib/buildkite-agent/builds/llvm-project/llvm/lib/Option/OptTable.cpp:70:15: error: no matching function for call to 'StrCmpOptionName'
  if (int N = StrCmpOptionName(A.Name, B.Name))
              ^~~~~~~~~~~~~~~~
/var/lib/buildkite-agent/builds/llvm-project/llvm/lib/Option/OptTable.cpp:60:12: note: candidate function not viable: no known conversion from 'const llvm::StringRef' to 'const char *' for 1st argument
static int StrCmpOptionName(const char *A, const char *B) {
           ^
1 error generated.

[serge-sans-paille]

fix test / compile errors

[nikic]

llvm/lib/Option/OptTable.cpp

38–39

The code in this function (and also StrCmpOptionName) depends on the terminating null byte, while a StringRef is not guaranteed to by directly followed by one. I think these two functions would have to be adjusted to operate on StringRef rather than pointers as well.

[serge-sans-paille]

Also simplify StrCmpOptionNameIgnoreCase implementation thanks to the move to StringRef

[nikic]

LG

llvm/lib/Option/OptTable.cpp

41

res -> Res

[thakis]

This breaks building lldb: http://45.33.8.238/linux/93366/step_4.txt

[thakis]

Reverted in de4b6a1bc64db33643f001ad45fae7b92b4a4688 for now. (Note that there was a follow-up fix in e50a60d7349de151bd2b06d85a79201ebc372d8a that I reverted in 13e061e73e1b067589d3bd680fc29297d7e4ec8d. When you reland, you'll likely want to cherry-pick that into your reland.)

[serge-sans-paille]

In D139274#3974176, @thakis wrote:

Reverted in de4b6a1bc64db33643f001ad45fae7b92b4a4688 for now. (Note that there was a follow-up fix in e50a60d7349de151bd2b06d85a79201ebc372d8a that I reverted in 13e061e73e1b067589d3bd680fc29297d7e4ec8d. When you reland, you'll likely want to cherry-pick that into your reland.)

You've been quicker than I am. Thanks for the revert.

[dyung]

@serge-sans-paille Your change is causing 24 test failures on the Linux PS4 bot, mostly with an assertion failure it seems. Can you take a look?

https://lab.llvm.org/buildbot/#/builders/139/builds/32263

clang/lib/Driver/ToolChains/Gnu.cpp

 
 static bool getStaticPIE(const ArgList &Args, const ToolChain &TC) {
   bool HasStaticPIE = Args.hasArg(options::OPT_static_pie);
   // -no-pie is an alias for -nopie. So, handling -nopie takes care of
   // -no-pie as well.
   if (HasStaticPIE && Args.hasArg(options::OPT_nopie)) {
     const Driver &D = TC.getDriver();
     const llvm::opt::OptTable &Opts = D.getOpts();
-    const char *StaticPIEName = Opts.getOptionName(options::OPT_static_pie);
-    const char *NoPIEName = Opts.getOptionName(options::OPT_nopie);
+    StringRef StaticPIEName = Opts.getOptionName(options::OPT_static_pie);
+    StringRef NoPIEName = Opts.getOptionName(options::OPT_nopie);
     D.Diag(diag::err_drv_cannot_mix_options) << StaticPIEName << NoPIEName;
   }
   return HasStaticPIE;
 }
 
 static bool getStatic(const ArgList &Args) {
   return Args.hasArg(options::OPT_static) &&
       !Args.hasArg(options::OPT_static_pie);

llvm/include/llvm/Option/OptTable.h

 /// parts of the driver still use Option instances where convenient.
 class OptTable {
 public:
   /// Entry for a single option instance in the option data table.
   struct Info {
     /// A null terminated array of prefix strings to apply to name while
     /// matching.
     const char *const *Prefixes;
-    const char *Name;
+    StringRef Name;
     const char *HelpText;
     const char *MetaVar;
     unsigned ID;
     unsigned char Kind;
     unsigned char Param;
     unsigned int Flags;
     unsigned short GroupID;
     unsigned short AliasID;
public:
 
   /// Get the given Opt's Option instance, lazily creating it
   /// if necessary.
   ///
   /// \return The option, or null for the INVALID option id.
   const Option getOption(OptSpecifier Opt) const;
 
   /// Lookup the name of the given option.
-  const char *getOptionName(OptSpecifier id) const {
-    return getInfo(id).Name;
-  }
+  StringRef getOptionName(OptSpecifier id) const { return getInfo(id).Name; }
 
   /// Get the kind of the given option.
   unsigned getOptionKind(OptSpecifier id) const {
     return getInfo(id).Kind;
   }
 
   /// Get the group id for the given option.
   unsigned getOptionGroupID(OptSpecifier id) const {
public:
   ///
   /// \param [in] Option - Prefix + Name of the flag which Values will be
   ///  changed. For example, "-analyzer-checker".
   /// \param [in] Values - String of Values seperated by ",", such as
   ///  "foo, bar..", where foo and bar is the argument which the Option flag
   ///  takes
   ///
   /// \return true in success, and false in fail.
-  bool addValues(const char *Option, const char *Values);
+  bool addValues(StringRef Option, const char *Values);
 
   /// Parse a single argument; returning the new argument and
   /// updating Index.
   ///
   /// \param [in,out] Index - The current parsing position in the argument
   /// string list; on return this will be the index of the next argument
   /// string to parse.
   /// \param [in] FlagsToInclude - Only parse options with any of these flags.

llvm/lib/Option/OptTable.cpp

 using namespace llvm;
 using namespace llvm::opt;
 
 namespace llvm {
 namespace opt {
 
 // Ordering on Info. The ordering is *almost* case-insensitive lexicographic,
 // with an exception. '\0' comes at the end of the alphabet instead of the
 // beginning (thus options precede any other options which prefix them).
-static int StrCmpOptionNameIgnoreCase(const char *A, const char *B) {
-  const char *X = A, *Y = B;
-  char a = tolower(*A), b = tolower(*B);
-  while (a == b) {
-    if (a == '\0')
-      return 0;
-
-    a = tolower(*++X);
-    b = tolower(*++Y);
-  }
+static int StrCmpOptionNameIgnoreCase(StringRef A, StringRef B) {
+  size_t MinSize = std::min(A.size(), B.size());
+  if (int Res = A.substr(0, MinSize).compare_insensitive(B.substr(0, MinSize)))
+    return Res;
 
-  if (a == '\0') // A is a prefix of B.
-    return 1;
-  if (b == '\0') // B is a prefix of A.
-    return -1;
+  if (A.size() == B.size())
+    return 0;
 
-  // Otherwise lexicographic.
-  return (a < b) ? -1 : 1;
+  return (A.size() == MinSize) ? 1 /* A is a prefix of B. */
+                               : -1 /* B is a prefix of A */;
 }
 
 #ifndef NDEBUG
-static int StrCmpOptionName(const char *A, const char *B) {
+static int StrCmpOptionName(StringRef A, StringRef B) {
   if (int N = StrCmpOptionNameIgnoreCase(A, B))
     return N;
-  return strcmp(A, B);
+  return A.compare(B);
 }
 
 static inline bool operator<(const OptTable::Info &A, const OptTable::Info &B) {
   if (&A == &B)
     return false;
 
   if (int N = StrCmpOptionName(A.Name, B.Name))
     return N < 0;
static inline bool operator<(const OptTable::Info &A, const OptTable::Info &B) {
   // should be joined, and it should succeed the other.
   assert(((A.Kind == Option::JoinedClass) ^ (B.Kind == Option::JoinedClass)) &&
          "Unexpected classes for options with same name.");
   return B.Kind == Option::JoinedClass;
 }
 #endif
 
 // Support lower_bound between info and an option name.
-static inline bool operator<(const OptTable::Info &I, const char *Name) {
+static inline bool operator<(const OptTable::Info &I, StringRef Name) {
   return StrCmpOptionNameIgnoreCase(I.Name, Name) < 0;
 }
 
 } // end namespace opt
 } // end namespace llvm
 
 OptSpecifier::OptSpecifier(const Option *Opt) : ID(Opt->getID()) {}
 
for (int P = 0;
         BestDistance = Distance;
         NearestString = (Candidate + RHS).str();
       }
     }
   }
   return BestDistance;
 }
 
-bool OptTable::addValues(const char *Option, const char *Values) {
+bool OptTable::addValues(StringRef Option, const char *Values) {
   for (size_t I = FirstSearchableIndex, E = OptionInfos.size(); I < E; I++) {
     Info &In = OptionInfos[I];
     if (optionMatches(In, Option)) {
       In.Values = Values;
       return true;
     }
   }
   return false;
std::unique_ptr<Arg> OptTable::parseOneArgGrouped(InputArgList &Args,
   // itself.
   const char *CStr = Args.getArgString(Index);
   StringRef Str(CStr);
   if (isInput(PrefixesUnion, Str))
     return std::make_unique<Arg>(getOption(InputOptionID), Str, Index++, CStr);
 
   const Info *End = OptionInfos.data() + OptionInfos.size();
   StringRef Name = Str.ltrim(PrefixChars);
-  const Info *Start = std::lower_bound(
-      OptionInfos.data() + FirstSearchableIndex, End, Name.data());
+  const Info *Start =
+      std::lower_bound(OptionInfos.data() + FirstSearchableIndex, End, Name);
   const Info *Fallback = nullptr;
   unsigned Prev = Index;
 
   // Search for the option which matches Str.
   for (; Start != End; ++Start) {
     unsigned ArgSize = matchOption(Start, Str, IgnoreCase);
     if (!ArgSize)
       continue;
std::unique_ptr<Arg> OptTable::ParseOneArg(const ArgList &Args, unsigned &Index,
   if (isInput(PrefixesUnion, Str))
     return std::make_unique<Arg>(getOption(InputOptionID), Str, Index++, Str);
 
   const Info *Start = OptionInfos.data() + FirstSearchableIndex;
   const Info *End = OptionInfos.data() + OptionInfos.size();
   StringRef Name = StringRef(Str).ltrim(PrefixChars);
 
   // Search for the first next option which could be a prefix.
-  Start = std::lower_bound(Start, End, Name.data());
+  Start = std::lower_bound(Start, End, Name);
 
   // Options are stored in sorted order, with '\0' at the end of the
   // alphabet. Since the only options which can accept a string must
   // prefix it, we iteratively search for the next option which could
   // be a prefix.
   //
   // FIXME: This is searching much more than necessary, but I am
   // blanking on the simplest way to make it fast. We can solve this

llvm/unittests/Option/OptionMarshallingTest.cpp

 //===- unittest/Support/OptionMarshallingTest.cpp - OptParserEmitter tests ===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/ADT/StringRef.h"
 #include "gtest/gtest.h"
 
 struct OptionWithMarshallingInfo {
-  const char *Name;
+  llvm::StringRef Name;
   const char *KeyPath;
   const char *ImpliedCheck;
   const char *ImpliedValue;
 };
 
 static const OptionWithMarshallingInfo MarshallingTable[] = {
 #define OPTION_WITH_MARSHALLING(                                               \
     PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,        \
     HELPTEXT, METAVAR, VALUES, SPELLING, SHOULD_PARSE, ALWAYS_EMIT, KEYPATH,   \
     DEFAULT_VALUE, IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER,     \
     MERGER, EXTRACTOR, TABLE_INDEX)                                            \
   {NAME, #KEYPATH, #IMPLIED_CHECK, #IMPLIED_VALUE},
 #include "Opts.inc"
 #undef OPTION_WITH_MARSHALLING
 };
 
 TEST(OptionMarshalling, EmittedOrderSameAsDefinitionOrder) {
-  ASSERT_STREQ(MarshallingTable[0].Name, "marshalled-flag-d");
-  ASSERT_STREQ(MarshallingTable[1].Name, "marshalled-flag-c");
-  ASSERT_STREQ(MarshallingTable[2].Name, "marshalled-flag-b");
-  ASSERT_STREQ(MarshallingTable[3].Name, "marshalled-flag-a");
+  ASSERT_STREQ(MarshallingTable[0].Name.data(), "marshalled-flag-d");
+  ASSERT_STREQ(MarshallingTable[1].Name.data(), "marshalled-flag-c");
+  ASSERT_STREQ(MarshallingTable[2].Name.data(), "marshalled-flag-b");
+  ASSERT_STREQ(MarshallingTable[3].Name.data(), "marshalled-flag-a");
 }
 
 TEST(OptionMarshalling, EmittedSpecifiedKeyPath) {
   ASSERT_STREQ(MarshallingTable[0].KeyPath, "X->MarshalledFlagD");
   ASSERT_STREQ(MarshallingTable[1].KeyPath, "X->MarshalledFlagC");
   ASSERT_STREQ(MarshallingTable[2].KeyPath, "X->MarshalledFlagB");
   ASSERT_STREQ(MarshallingTable[3].KeyPath, "X->MarshalledFlagA");
 }

llvm/utils/TableGen/OptParserEmitter.cpp

 
 static std::string getOptionSpelling(const Record &R) {
   size_t PrefixLength;
   return getOptionSpelling(R, PrefixLength);
 }
 
 static void emitNameUsingSpelling(raw_ostream &OS, const Record &R) {
   size_t PrefixLength;
-  OS << "&";
+  OS << "llvm::StringRef(";
   write_cstring(OS, StringRef(getOptionSpelling(R, PrefixLength)));
-  OS << "[" << PrefixLength << "]";
+  OS << ").substr(" << PrefixLength << ")";
 }
 
 class MarshallingInfo {
 public:
   static constexpr const char *MacroName = "OPTION_WITH_MARSHALLING";
   const Record &R;
   bool ShouldAlwaysEmit;
   StringRef MacroPrefix;