[CommandLine] Remove OptionCategory and SubCommand caches from the Option class.

[hintonda]

Summary

This change processes OptionCategorys and SubCommands as they
are seen instead of caching them in the Option class and processing
them later. Doing so simplifies the work needed to be done by the Global
parser and significantly reduces the size of the Option class to a mere 64
bytes.

Removing the OptionCategory cache saved 24 bytes, and removing
the SubCommand cache saved an additional 48 bytes, for a total of a
72 byte reduction.

[hintonda]

The Option class started at 184 bytes, this change, along with D62091, gets it down to 88 types, and reduces bin/opt by over 90k.

[hintonda]

• Add comments, fix auto usage, and minor refactoring.

[hintonda]

• Remove Categories cache as well. This change reduces the size of the Option class down to 64 bytes.

[beanz]

Adding @zturner in case he has any thoughts.

[serge-sans-paille]

llvm/include/llvm/Support/CommandLine.h

309

Why do you need to make that getter public?

[hintonda]

llvm/include/llvm/Support/CommandLine.h

309

I think at some point I needed access, but as of now, FullyInitialized is only used within Option, so it can go away.

Thanks for catching that.

[hintonda]

• Removed unneeded public method from Option.

[hintonda]

ping...

[serge-sans-paille]

LGTM

[hintonda]

Since a SubCommand's OptionsMap key is a string, and multiple
SubCommands can have unrelated options of the same name,
getSubCommands needs to also make sure the found Option matches the
this pointer.

Also, hid the addArgument() method in Alias to prevent users from
trying to add it twice, once when specifying a SubCommand, and again
in the done() method.

Added tests to cover these two cases -- first discovered in llvm-xray,
which declares duplicate Options and Aliases in multiple SubCommands.

[hintonda]

llvm/include/llvm/Support/CommandLine.h

1796

Will fix this and related methods by making them virtual and adding asserts saying the operation isn't allowed/doesn't make sense in a separate patch.

[MaskRay]

llvm/include/llvm/Support/CommandLine.h

290

16. Does it have to be so large?

[hintonda]

llvm/include/llvm/Support/CommandLine.h

290

No, it should probably be 1 or 2, and getSubCommands should probably be 1.

Thanks for pointing this out.

[hintonda]

• Reduce compile-time container sizes.

[hintonda]

• Make Option::addArgument virtual.

[hintonda]

Even though this patch has been accepted, I've made 2 additional changes, and would
like to have them reviewed as well.

The first just reduces the compile-time default container sizes for the get* methods,
which shouldn't be too controversial.

However, the second is a bit more involved, and can be viewed in the following two
diffs -- the second of which just makes addArgument virtual.

https://reviews.llvm.org/D62105?vs=201049&id=202622&whitespace=ignore-most#toc
https://reviews.llvm.org/D62105?vs=204984&id=205092&whitespace=ignore-most#toc

It fixes bugs in the following two use cases (please see the two additional uinittests that
cover these):

• When a users incorrectly passes a cl::sub argument to an Alias ctor:
  
  Since Alias's use the SubCommands of the Option they alias, this argument should be ignored, or assert.

• When using SubCommands with duplicate Alias's, within the scope of a SubCommand:
  
  In order to distinguish which SubCommand the matching Alias belongs, it is necessary to compare the found option to this.

[serge-sans-paille]

llvm/include/llvm/Support/CommandLine.h

1796

Why do you want to do that in a separate patch?

[hintonda]

llvm/include/llvm/Support/CommandLine.h

1796

Sorry, that comment is OBE. I made it virtual instead.

My original reasoning was to make each patch smaller, but found breaking it up further un-workable.

[hintonda]

Reopening to track fix for buildbot failure -- need to make GeneralCategory a ManagedStatic.

[hintonda]

Patch is ready, so I'll post as soon as I complete rebase/build/test.

The fix is just making GeneralCategory a ManagedStatic, so without objection, I'll land it tomorrow and watch the bots.

[hintonda]

Make GeneralCategory a ManagedStatic.

llvm/include/llvm/Support/CommandLine.h

class Option {
   uint16_t AdditionalVals;       // Greater than 0 for multi-valued option.
 
 public:
   StringRef ArgStr;   // The argument string itself (ex: "help", "o")
   StringRef HelpStr;  // The descriptive text message for -help
   StringRef ValueStr; // String describing what the value of this option is
   SmallVector<OptionCategory *, 1>
       Categories;                    // The Categories this option belongs to
-  SmallPtrSet<SubCommand *, 1> Subs; // The subcommands this option belongs to.
+
+  // Return the set of SubCommands that this Option was added to.
+  SmallPtrSet<SubCommand *, 4> getSubCommands() const;
 
   inline enum NumOccurrencesFlag getNumOccurrencesFlag() const {
     return (enum NumOccurrencesFlag)Occurrences;
   }
 
   inline enum ValueExpected getValueExpectedFlag() const {
     return Value ? ((enum ValueExpected)Value) : getValueExpectedFlagDefault();
   }
 
   inline enum OptionHidden getOptionHiddenFlag() const {
     return (enum OptionHidden)HiddenFlag;
   }
 
   inline enum FormattingFlags getFormattingFlag() const {
     return (enum FormattingFlags)Formatting;
   }
 
   inline unsigned getMiscFlags() const { return Misc; }
+  inline bool isFullyInitialized() const { return FullyInitialized; }
   inline unsigned getPosition() const { return Position; }
   inline unsigned getNumAdditionalVals() const { return AdditionalVals; }
 
   // hasArgStr - Return true if the argstr != ""
   bool hasArgStr() const { return !ArgStr.empty(); }
   bool isPositional() const { return getFormattingFlag() == cl::Positional; }
   bool isSink() const { return getMiscFlags() & cl::Sink; }
   bool isDefaultOption() const { return getMiscFlags() & cl::DefaultOption; }
 
   bool isConsumeAfter() const {
     return getNumOccurrencesFlag() == cl::ConsumeAfter;
   }
 
-  bool isInAllSubCommands() const {
-    return any_of(Subs, [](const SubCommand *SC) {
-      return SC == &*AllSubCommands;
-    });
-  }
-
   //-------------------------------------------------------------------------===
   // Accessor functions set by OptionModifiers
   //
   void setArgStr(StringRef S);
   void setDescription(StringRef S) { HelpStr = S; }
   void setValueStr(StringRef S) { ValueStr = S; }
   void setNumOccurrencesFlag(enum NumOccurrencesFlag Val) { Occurrences = Val; }
   void setValueExpectedFlag(enum ValueExpected Val) { Value = Val; }
   void setHiddenFlag(enum OptionHidden Val) { HiddenFlag = Val; }
   void setFormattingFlag(enum FormattingFlags V) { Formatting = V; }
   void setMiscFlag(enum MiscFlags M) { Misc |= M; }
   void setPosition(unsigned pos) { Position = pos; }
   void addCategory(OptionCategory &C);
-  void addSubCommand(SubCommand &S) { Subs.insert(&S); }
 
 protected:
   explicit Option(enum NumOccurrencesFlag OccurrencesFlag,
                   enum OptionHidden Hidden)
       : NumOccurrences(0), Occurrences(OccurrencesFlag), Value(0),
         HiddenFlag(Hidden), Formatting(NormalFormatting), Misc(0),
         FullyInitialized(false), Position(0), AdditionalVals(0) {
     Categories.push_back(&GeneralCategory);
   }
 
   inline void setNumAdditionalVals(unsigned n) { AdditionalVals = n; }
 
 public:
   virtual ~Option() = default;
 
   // addArgument - Register this argument with the commandline system.
   //
-  void addArgument();
+  void addArgument(SubCommand &SC);
+
+  // addArgument - Only called in done() method to add default
+  // TopLevelSubCommand.
+  void addArgument() {
+    if (!isFullyInitialized())
+      addArgument(*TopLevelSubCommand);
+  }
 
   /// Unregisters this option from the CommandLine system.
   ///
   /// This option must have been the last option registered.
   /// For testing purposes only.
   void removeArgument();
 
   // Return the width of the option tag for printing...

 };
 
 // sub - Specify the subcommand that this option belongs to.
 struct sub {
   SubCommand &Sub;
 
   sub(SubCommand &S) : Sub(S) {}
 
-  template <class Opt> void apply(Opt &O) const { O.addSubCommand(Sub); }
+  template <class Opt> void apply(Opt &O) const { O.addArgument(Sub); }
 };
 
 //===----------------------------------------------------------------------===//
 // OptionValue class
 
 // Support value comparison outside the template.
 struct GenericOptionValue {
   virtual bool compare(const GenericOptionValue &V) const = 0;
ValueExpected getValueExpectedFlagDefault() const override {
     return AliasFor->getValueExpectedFlag();
   }
 
   void done() {
     if (!hasArgStr())
       error("cl::alias must have argument name specified!");
     if (!AliasFor)
       error("cl::alias must have an cl::aliasopt(option) specified!");
-    if (!Subs.empty())
-      error("cl::alias must not have cl::sub(), aliased option's cl::sub() will be used!");
-    Subs = AliasFor->Subs;
     Categories = AliasFor->Categories;
-    addArgument();
+    for(SubCommand *SC: AliasFor->getSubCommands())
+      addArgument(*SC);
   }
 
 public:
   // Command line options should not be copyable
   alias(const alias &) = delete;
   alias &operator=(const alias &) = delete;
 
   void setAliasFor(Option &O) {
     if (AliasFor)
       error("cl::alias must only have one cl::aliasopt(...) specified!");
     AliasFor = &O;
   }
 
   template <class... Mods>
   explicit alias(const Mods &... Ms)
       : Option(Optional, Hidden), AliasFor(nullptr) {
     apply(this, Ms...);
     done();
   }
 };
 
 // aliasfor - Modifier to set the option an alias aliases.
 struct aliasopt {
   Option &Opt;

llvm/lib/Support/CommandLine.cpp

public:
   StringRef ProgramOverview;
 
   // This collects additional help to be printed.
   std::vector<StringRef> MoreHelp;
 
   // This collects Options added with the cl::DefaultOption flag. Since they can
   // be overridden, they are not added to the appropriate SubCommands until
   // ParseCommandLineOptions actually runs.
-  SmallVector<Option*, 4> DefaultOptions;
+  SmallVector<std::pair<Option*, SubCommand*>, 4> DefaultOptions;
 
   // This collects the different option categories that have been registered.
   SmallPtrSet<OptionCategory *, 16> RegisteredOptionCategories;
 
   // This collects the different subcommands that have been registered.
   SmallPtrSet<SubCommand *, 4> RegisteredSubCommands;
 
   CommandLineParser() : ActiveSubCommand(nullptr) {
if (SC == &*AllSubCommands) {
         if (SC == Sub)
           continue;
         addLiteralOption(Opt, Sub, Name);
       }
     }
   }
 
   void addLiteralOption(Option &Opt, StringRef Name) {
-    if (Opt.Subs.empty())
-      addLiteralOption(Opt, &*TopLevelSubCommand, Name);
-    else {
-      for (auto SC : Opt.Subs)
-        addLiteralOption(Opt, SC, Name);
-    }
+    for(SubCommand *SC: Opt.getSubCommands())
+
+  void addOption(Option *O, SubCommand *SC, bool ProcessDefaultOptions = false) {
+    if (!ProcessDefaultOptions && O->isDefaultOption()) {
+      DefaultOptions.push_back(std::make_pair(O, SC));
+      return;
     }
 
-  void addOption(Option *O, SubCommand *SC) {
     bool HadErrors = false;
     if (O->hasArgStr()) {
       // If it's a DefaultOption, check to make sure it isn't already there.
       if (O->isDefaultOption() &&
           SC->OptionsMap.find(O->ArgStr) != SC->OptionsMap.end())
         return;
 
       // Add argument to the argument map!
if (HadErrors)
       report_fatal_error("inconsistency in registered CommandLine options");
 
     // If we're adding this to all sub-commands, add it to the ones that have
     // already been registered.
     if (SC == &*AllSubCommands) {
       for (const auto &Sub : RegisteredSubCommands) {
         if (SC == Sub)
           continue;
-        addOption(O, Sub);
+        addOption(O, Sub, ProcessDefaultOptions);
       }
     }
   }
 
-  void addOption(Option *O, bool ProcessDefaultOption = false) {
-    if (!ProcessDefaultOption && O->isDefaultOption()) {
-      DefaultOptions.push_back(O);
-      return;
-    }
-
-    if (O->Subs.empty()) {
-      addOption(O, &*TopLevelSubCommand);
-    } else {
-      for (auto SC : O->Subs)
-        addOption(O, SC);
+  void addDefaultOptions() {
+    for (std::pair<Option *, SubCommand *> &DO : DefaultOptions) {
+      addOption(DO.first, DO.second, true);
     }
   }
 
   void removeOption(Option *O, SubCommand *SC) {
     SmallVector<StringRef, 16> OptionNames;
     O->getExtraOptionNames(OptionNames);
     if (O->hasArgStr())
       OptionNames.push_back(O->ArgStr);
else if (O->getMiscFlags() & cl::Sink)
           break;
         }
       }
     else if (O == Sub.ConsumeAfterOpt)
       Sub.ConsumeAfterOpt = nullptr;
   }
 
   void removeOption(Option *O) {
-    if (O->Subs.empty())
-      removeOption(O, &*TopLevelSubCommand);
-    else {
-      if (O->isInAllSubCommands()) {
-        for (auto SC : RegisteredSubCommands)
-          removeOption(O, SC);
-      } else {
-        for (auto SC : O->Subs)
-          removeOption(O, SC);
-      }
-    }
   }
 
   bool hasOptions(const SubCommand &Sub) const {
     return (!Sub.OptionsMap.empty() || !Sub.PositionalOpts.empty() ||
             nullptr != Sub.ConsumeAfterOpt);
   }
 
   bool hasOptions() const {
if (!Sub.OptionsMap.insert(std::make_pair(NewName, O)).second) {
       errs() << ProgramName << ": CommandLine Error: Option '" << O->ArgStr
              << "' registered more than once!\n";
       report_fatal_error("inconsistency in registered CommandLine options");
     }
     Sub.OptionsMap.erase(O->ArgStr);
   }
 
   void updateArgStr(Option *O, StringRef NewName) {
-    if (O->Subs.empty())
-      updateArgStr(O, NewName, &*TopLevelSubCommand);
-    else {
-      if (O->isInAllSubCommands()) {
-        for (auto SC : RegisteredSubCommands)
-          updateArgStr(O, NewName, SC);
-      } else {
-        for (auto SC : O->Subs)
-          updateArgStr(O, NewName, SC);
-      }
-    }
   }
 
   void printOptionValues();
 
   void registerCategory(OptionCategory *cat) {
     assert(count_if(RegisteredOptionCategories,
                     [cat](const OptionCategory *Category) {
              return cat->getName() == Category->getName();

 void cl::AddLiteralOption(Option &O, StringRef Name) {
   GlobalParser->addLiteralOption(O, Name);
 }
 
 extrahelp::extrahelp(StringRef Help) : morehelp(Help) {
   GlobalParser->MoreHelp.push_back(Help);
 }
 
-void Option::addArgument() {
-  GlobalParser->addOption(this);
+void Option::addArgument(SubCommand &SC) {
+  GlobalParser->addOption(this, &SC);
   FullyInitialized = true;
 }
 
 void Option::removeArgument() { GlobalParser->removeOption(this); }
 
+SmallPtrSet<SubCommand *, 4> Option::getSubCommands() const {
+  // This can happen for enums and literal options.
+  if (ArgStr.empty())
+    return SmallPtrSet<SubCommand *, 4>{&*TopLevelSubCommand};
+
+  SmallPtrSet<SubCommand *, 4> Subs;
+  for (SubCommand *SC : GlobalParser->getRegisteredSubcommands()) {
+    auto I = SC->OptionsMap.find(ArgStr);
+    if (I != SC->OptionsMap.end())
+      Subs.insert(SC);
+  }
+  return Subs;
+}
+
 void Option::setArgStr(StringRef S) {
   if (FullyInitialized)
     GlobalParser->updateArgStr(this, S);
   assert((S.empty() || S[0] != '-') && "Option can't start with '-");
   ArgStr = S;
   if (ArgStr.size() == 1)
     setMiscFlag(Grouping);
 }
bool CommandLineParser::ParseCommandLineOptions(int argc,
   GlobalParser->ActiveSubCommand = ChosenSubCommand;
 
   assert(ChosenSubCommand);
   auto &ConsumeAfterOpt = ChosenSubCommand->ConsumeAfterOpt;
   auto &PositionalOpts = ChosenSubCommand->PositionalOpts;
   auto &SinkOpts = ChosenSubCommand->SinkOpts;
   auto &OptionsMap = ChosenSubCommand->OptionsMap;
 
-  for (auto O: DefaultOptions) {
-    addOption(O, true);
-  }
+  addDefaultOptions();
 
   if (ConsumeAfterOpt) {
     assert(PositionalOpts.size() > 0 &&
            "Cannot specify cl::ConsumeAfter without a positional argument!");
   }
   if (!PositionalOpts.empty()) {
 
     // Calculate how many positional values are _required_.