This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by hintonda on May 18 2019, 9:59 PM.

Event Timeline

hintonda created this revision.May 18 2019, 9:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2019, 9:59 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

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 retitled this revision from [CommandLine} Remove SubCommands SmallPtrSet from the Option class. to [CommandLine] Remove SubCommands SmallPtrSet from the Option class..May 18 2019, 10:10 PM
hintonda updated this revision to Diff 200183.May 19 2019, 10:49 AM
  • Add comments, fix auto usage, and minor refactoring.
hintonda updated this revision to Diff 200185.May 19 2019, 1:23 PM
  • Remove Categories cache as well. This change reduces the size of the Option class down to 64 bytes.
hintonda retitled this revision from [CommandLine] Remove SubCommands SmallPtrSet from the Option class. to [CommandLine] Remove OptionCategory and SubCommand caches from the Option class..May 19 2019, 1:28 PM
hintonda edited the summary of this revision. (Show Details)
hintonda edited the summary of this revision. (Show Details)
beanz added a subscriber: zturner.

Adding @zturner in case he has any thoughts.

tstellar added a subscriber: tstellar.
llvm/include/llvm/Support/CommandLine.h
313

Why do you need to make that getter public?

hintonda marked an inline comment as done.May 23 2019, 11:04 AM
hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
313

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 updated this revision to Diff 201049.May 23 2019, 1:03 PM
  • Removed unneeded public method from Option.
This revision is now accepted and ready to land.May 31 2019, 9:55 AM
hintonda updated this revision to Diff 202622.EditedJun 2 2019, 1:37 PM

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 marked an inline comment as done.Jun 3 2019, 8:17 AM
hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
1798

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 added inline comments.Jun 3 2019, 11:05 PM
llvm/include/llvm/Support/CommandLine.h
291
  1. Does it have to be so large?
hintonda marked an inline comment as done.Jun 5 2019, 6:55 PM
hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
291

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

Thanks for pointing this out.

hintonda updated this revision to Diff 204984.Jun 16 2019, 6:42 PM
  • Reduce compile-time container sizes.
hintonda updated this revision to Diff 205092.Jun 17 2019, 8:55 AM
  • Make Option::addArgument virtual.

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.
llvm/include/llvm/Support/CommandLine.h
1798

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

hintonda marked an inline comment as done.Jun 17 2019, 10:06 AM
hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
1798

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.

This revision was automatically updated to reflect the committed changes.
hintonda reopened this revision.Jul 9 2019, 7:26 AM

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

This revision is now accepted and ready to land.Jul 9 2019, 7:26 AM

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 updated this revision to Diff 208736.Jul 9 2019, 10:31 AM

Make GeneralCategory a ManagedStatic.

This revision was automatically updated to reflect the committed changes.