Groups keyboard shortcuts have been updated
Dismiss
See shortcuts

[LLVMdev] [RFC] Removing static initializers for command line options

231 views
Skip to first unread message

Chris Bieneman

unread,
Aug 18, 2014, 2:51:30 PM8/18/14
to LLVM Dev
Today command line arguments in LLVM are global variables. An example argument from Scalarizer.cpp is:

static cl::opt<bool> ScalarizeLoadStore
  ("scalarize-load-store", cl::Hidden, cl::init(false),
   cl::desc("Allow the scalarizer pass to scalarize loads and store"));

This poses a problem for clients of LLVM that aren’t traditional compilers (i.e. WebKit, and Mesa). My proposal is to take a phased approach at addressing this issue.

The first phase is to move the ownership of command line options to a singleton, OptionRegistry. The OptionRegistry can be made to work with the existing global command line definitions so that the changes to migrate options can be done in small batches. The primary purpose of this change is to move the ownership of the command line options out of the global scope, and to provide a vehicle for threading them through the compiler. At the completion of this phase, all the command line arguments will be constructed during LLVM initialization and registered under the OptionRegistry. This will replace the 100’s of static initialized cl::opt objects with a single static initialized OptionRegistry.

With this change options can be constructed during initialization. For the example option above the pass initialization would get a line like:

cl::OptionRegistry::CreateOption<bool>("ScalarizeLoadStore",
  "scalarize-load-store", cl::Hidden, cl::init(false),
  cl::desc("Allow the scalarizer pass to scalarize loads and store"));

Also the pass would add a boolean member to store the value of the option which would be initialized in the pass’s constructor like this:

ScalarizeLoadStore = cl::OptionRegistry::GetValue<bool>("ScalarizeLoadStore");

These two operations need to occur at separate times due to object lifespans. At the time when command lines are parsed passes have been initialized, but not constructed. That means making options live in passes doesn’t really work, but since we want the data to be part of the pass we need to initialize it during construction.

A large part of this phase will be finding appropriate places for all the command line options to be initialized, and identifying all the places where the option data will need to be threaded through the compiler. One of the goals here is to get rid of all global state in the compiler to (eventually) enable better multi-threading by clients like WebKit.

The second phase is to split the OptionRegistry into two pieces. The first piece is the parsing logic, and the second piece is the Option data store. The main goal of this phase is to make the OptionRegistry represent everything needed to parse command line options and to define a new second object, OptionStore, that is populated with values by parsing the command line. The OptionRegistry will be responsible for initializing “blank” option stores which can then be populated by either the command line parser, or API calls.

The OptionRegistry should remain a singleton so that the parsing logic for all options remains universally available. This is required to continue supporting plugin loadable options.

The OptionStore should be created when a command line is parsed, or by an API call in libraries, and can be passed through the pass manager and targets to populate option data. The OptionStore should have a lifetime independent of contexts, and pass managers because it can be re-used indiscriminately.

The core principle in this design is that the objects involved in parsing options only need to exist once, but you need a full list of all options in order to parse a command line. You should be able to have multiple copies of the actual stored option data. Having multiple copies of the data store is one step toward enabling two instances of LLVM in the same process to use optimization passes with different options.

I haven’t come up with a specific implementation proposal for this yet, but I do have some rough ideas. The basic flow that I’m thinking of is for command line parsing to create an object that maps option names to their values without any of the parsing data involved. This would allow for either parsing multiple command lines, or generally just constructing multiple option data stores. **Here is where things get foggy because I haven’t yet looked too deep.** Once you construct a data store it will get passed into the pass manager (and everywhere else that needs it), and it will be used to initialize all the option values.

There has been discussion about making the option store reside within the context, but this doesn’t feel right because the biggest consumer of option data is the passes, and you can use a single pass manager with multiple contexts.

Thanks,
-Chris
cl_opt.diff

Rafael Espíndola

unread,
Aug 18, 2014, 5:47:47 PM8/18/14
to Chris Bieneman, LLVM Dev

For the first step it might be better to keep the option value as a
global. That way we only switch to using something like


static bool ScalarizeLoadStore;
cl::OptionRegistry::CreateOption<bool>(&ScalarizeLoadStore,


"ScalarizeLoadStore",
"scalarize-load-store", cl::Hidden, cl::init(false),
cl::desc("Allow the scalarizer pass to scalarize loads and store"));

and everything else remains as is.

Some passes take options directly in the constructor. For example

Inliner::Inliner(char &ID, int Threshold, bool InsertLifetime)

Maybe we could just say that there are two different types of options.
The ones we want to expose to users and the ones which we use for
testing llvm itself. The options we want to expose should be just
constructor arguments. With that distinction we should be able to just
not use the options added by cl::OptionRegistry::CreateOption unless
cl::ParseCommandLineOptions is called. WebKit like clients would just
not call cl::ParseCommandLineOptions. Would that work?

Cheers,
Rafael

_______________________________________________
LLVM Developers mailing list
LLV...@cs.uiuc.edu http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

Chris Bieneman

unread,
Aug 18, 2014, 6:00:49 PM8/18/14
to Rafael Espíndola, LLVM Dev
I’d prefer to do the removal of global storage all at once. This can be done one pass at a time, but doing all at once means I don’t need to revisit each pass as many times. Whereas if I do this in two passes (where the option becomes owned, but the storage remains global) I’ll need to revisit each pass again to get rid of the global storage.

Keep in mind one of the ultimate goals that this is building toward is allowing a single process to compile multiple programs at the same time with different options.
This is actually how some of our internal clients are already working. There are a few caveats with this approach:

(1) You can’t allow the pass manager to allocate your passes for you, because those passes only read from cl::opts
(2) Not all of our passes have constructors for overriding their cl::opts (the legacy Scalarizer is one)

I think it would in general be cleaner to provide a way for library clients to use cl::opts without being forced to parse a command line.

Thanks,
-Chris


Cheers,
Rafael

Rafael Espíndola

unread,
Aug 18, 2014, 6:13:56 PM8/18/14
to Chris Bieneman, LLVM Dev
> Some passes take options directly in the constructor. For example
>
> Inliner::Inliner(char &ID, int Threshold, bool InsertLifetime)
>
> Maybe we could just say that there are two different types of options.
> The ones we want to expose to users and the ones which we use for
> testing llvm itself. The options we want to expose should be just
> constructor arguments. With that distinction we should be able to just
> not use the options added by cl::OptionRegistry::CreateOption unless
> cl::ParseCommandLineOptions is called. WebKit like clients would just
> not call cl::ParseCommandLineOptions. Would that work?
>
>
> This is actually how some of our internal clients are already working. There
> are a few caveats with this approach:
>
> (1) You can’t allow the pass manager to allocate your passes for you,
> because those passes only read from cl::opts

You mean PassManagerBuilder, right?

> (2) Not all of our passes have constructors for overriding their cl::opts
> (the legacy Scalarizer is one)
>
> I think it would in general be cleaner to provide a way for library clients
> to use cl::opts without being forced to parse a command line.

I guess it really depends on how many options there are that we want
to expose via an API. I have the impression that there are few, which
would make changing the constructors and PassManagerBuilder better.

If there is a large number of options that we want to expose, then I
can see the value of having a llvm "configuration object" that is
passed around and is queried by the passes. If we do go down this
road, we should change passes like the inliner to use the
configuration object instead of constructor options. We should also
drop the "cl" from the names if it is not going to be handling command
lines :-)

Chris Bieneman

unread,
Aug 18, 2014, 6:25:50 PM8/18/14
to Rafael Espíndola, LLVM Dev

> On Aug 18, 2014, at 3:09 PM, Rafael Espíndola <rafael.e...@gmail.com> wrote:
>
>> Some passes take options directly in the constructor. For example
>>
>> Inliner::Inliner(char &ID, int Threshold, bool InsertLifetime)
>>
>> Maybe we could just say that there are two different types of options.
>> The ones we want to expose to users and the ones which we use for
>> testing llvm itself. The options we want to expose should be just
>> constructor arguments. With that distinction we should be able to just
>> not use the options added by cl::OptionRegistry::CreateOption unless
>> cl::ParseCommandLineOptions is called. WebKit like clients would just
>> not call cl::ParseCommandLineOptions. Would that work?
>>
>>
>> This is actually how some of our internal clients are already working. There
>> are a few caveats with this approach:
>>
>> (1) You can’t allow the pass manager to allocate your passes for you,
>> because those passes only read from cl::opts
>
> You mean PassManagerBuilder, right?

Yes.

>
>> (2) Not all of our passes have constructors for overriding their cl::opts
>> (the legacy Scalarizer is one)
>>
>> I think it would in general be cleaner to provide a way for library clients
>> to use cl::opts without being forced to parse a command line.
>
> I guess it really depends on how many options there are that we want
> to expose via an API. I have the impression that there are few, which
> would make changing the constructors and PassManagerBuilder better.
>
> If there is a large number of options that we want to expose, then I
> can see the value of having a llvm "configuration object" that is
> passed around and is queried by the passes. If we do go down this
> road, we should change passes like the inliner to use the
> configuration object instead of constructor options. We should also
> drop the "cl" from the names if it is not going to be handling command
> lines :-)

I’m curious if Tom Stellard or Filip Pizlo have any input on this as they are more directly involved on the client side.

I do agree that we should ultimately drop the cl namespace if we’re going in this direction.

-Chris

Filip Pizlo

unread,
Aug 18, 2014, 7:30:11 PM8/18/14
to Chris Bieneman, LLVM Dev

The fewer options we fiddle with, the better for WebKit. Hence we would be fine with a solution that exposes relatively few options.

The main option that we use now - turning on stack map liveness calculation - is something that feels like it shouldn't be an "option" at all but maybe an attribute instead.

Chris Bieneman

unread,
Aug 18, 2014, 7:32:42 PM8/18/14
to Filip Pizlo, LLVM Dev
How do you construct you PassManager?

-Chris

Keno Fischer

unread,
Aug 19, 2014, 2:19:05 AM8/19/14
to Chris Bieneman, LLVM Dev
I've recently experienced a similar pain with multiple
ExecutionEngines generating code (multiple LLVM-based JITs in the same
process). I don't have anything specific to add at this point, but I
wanted to express my whole hearted +1 on the project.

Filip Pizlo

unread,
Aug 19, 2014, 11:32:33 AM8/19/14
to Chris Bieneman, LLVM Dev
LLVMCreatePassManager() and then we add target data, add a target machine analysis pass, and then we construct our pipeline and call LLVMRunPassManager().

-Filip

Chris Bieneman

unread,
Aug 19, 2014, 1:49:44 PM8/19/14
to Filip Pizlo, LLVM Dev
I’d like to propose moving forward with the first phase of my proposal to make the cl::opt structures owned and eliminate global option storage. I’d also like to add to it that when updating passes I will ensure that each pass that has cl::opts also has a default constructor, an overridden constructor to populate each option, and the corresponding factory methods for the C API.

Does this sound reasonable for a first step?

-Chris

Rafael Espíndola

unread,
Aug 19, 2014, 2:12:52 PM8/19/14
to Chris Bieneman, LLVM Dev
On 19 August 2014 13:47, Chris Bieneman <be...@apple.com> wrote:
> I’d like to propose moving forward with the first phase of my proposal to
> make the cl::opt structures owned and eliminate global option storage.

For now, please eliminate only the static constructor and leave the
storage. Since it seems only a few options that need to be exposed by
non-command line APIs, we might be able to avoid the need for a
cl::OptionRegistry::GetValue.

> I’d
> also like to add to it that when updating passes I will ensure that each
> pass that has cl::opts also has a default constructor, an overridden
> constructor to populate each option, and the corresponding factory methods
> for the C API.

And *please* don't add anything to the C api unless someone has a real
need for that particular interface and there is a good discussion on
the review thread about it. The C api has more backwards compatibility
promises, which makes it incredibly painful :-(

Even the C++ api is not free, so I would also only modify a pass
constructor if someone wants to pass that option for something other
then llvm development or testing. For that command lines are a
perfectly reasonable solution.

> Does this sound reasonable for a first step?

Removing the static constructors does.

Filip Pizlo

unread,
Aug 19, 2014, 2:23:25 PM8/19/14
to Rafael Espíndola, LLVM Dev

> On Aug 19, 2014, at 11:09 AM, Rafael Espíndola <rafael.e...@gmail.com> wrote:
>
>> On 19 August 2014 13:47, Chris Bieneman <be...@apple.com> wrote:
>> I’d like to propose moving forward with the first phase of my proposal to
>> make the cl::opt structures owned and eliminate global option storage.
>
> For now, please eliminate only the static constructor and leave the
> storage. Since it seems only a few options that need to be exposed by
> non-command line APIs, we might be able to avoid the need for a
> cl::OptionRegistry::GetValue.
>
>> I’d
>> also like to add to it that when updating passes I will ensure that each
>> pass that has cl::opts also has a default constructor, an overridden
>> constructor to populate each option, and the corresponding factory methods
>> for the C API.
>
> And *please* don't add anything to the C api unless someone has a real
> need for that particular interface and there is a good discussion on
> the review thread about it. The C api has more backwards compatibility
> promises, which makes it incredibly painful :-(

This is a good point. I see two sensible options:

1) don't add anything to the C API unless someone specifically asks, as Rafael suggests.

2) make options passed to passes use some kind of loose coupling, like an array of strings or even better an options object where the user sets key/value pairs by some call (eg. LLVMSetOption(optionObject, keyString, valueString).

The upside of (2) is that it preserves current functionality and new options can be added easily. The downside is that we'd have to get very particular about whether an option needs to be supported indefinitely if it is ever exposed. Probably nobody wants that strong of a contract.

-Filip

Rafael Espíndola

unread,
Aug 19, 2014, 2:29:30 PM8/19/14
to Filip Pizlo, LLVM Dev
> This is a good point. I see two sensible options:
>
> 1) don't add anything to the C API unless someone specifically asks, as Rafael suggests.
>
> 2) make options passed to passes use some kind of loose coupling, like an array of strings or even better an options object where the user sets key/value pairs by some call (eg. LLVMSetOption(optionObject, keyString, valueString).
>
> The upside of (2) is that it preserves current functionality and new options can be added easily. The downside is that we'd have to get very particular about whether an option needs to be supported indefinitely if it is ever exposed. Probably nobody wants that strong of a contract.

This depends on what we mean by "ABI". Do me mean "it links"? If so,
yes, something like LLVMConfigSetBoolValue(Config,
"ScalarizeLoadStore", 1) is not part of the C abi. We could remove or
rename the ScalarizeLoadStore option.

But in my opinion that is a bit too narrow a definition. What we
really should care about is "WebKit still works with the next llvm".
The same is true for other users, and if one of those users depends on
an option, having that option be symbol, a constructor argument or or
a well know string seems like an implementation detail.

Chris Bieneman

unread,
Aug 19, 2014, 2:46:49 PM8/19/14
to Rafael Espíndola, LLVM Dev

> On Aug 19, 2014, at 11:09 AM, Rafael Espíndola <rafael.e...@gmail.com> wrote:
>
> On 19 August 2014 13:47, Chris Bieneman <be...@apple.com> wrote:
>> I’d like to propose moving forward with the first phase of my proposal to
>> make the cl::opt structures owned and eliminate global option storage.
>
> For now, please eliminate only the static constructor and leave the
> storage. Since it seems only a few options that need to be exposed by
> non-command line APIs, we might be able to avoid the need for a
> cl::OptionRegistry::GetValue.

I strongly feel this is the wrong decision. If you have a single process using two LLVM clients (say WebKit and Mesa), and they both are using an opt pass with different settings. If the storage is global this will not work.

>
>> I’d
>> also like to add to it that when updating passes I will ensure that each
>> pass that has cl::opts also has a default constructor, an overridden
>> constructor to populate each option, and the corresponding factory methods
>> for the C API.
>
> And *please* don't add anything to the C api unless someone has a real
> need for that particular interface and there is a good discussion on
> the review thread about it. The C api has more backwards compatibility
> promises, which makes it incredibly painful :-(
>
> Even the C++ api is not free, so I would also only modify a pass
> constructor if someone wants to pass that option for something other
> then llvm development or testing. For that command lines are a
> perfectly reasonable solution.

I can agree with all of this.

-Chris

Rafael Espíndola

unread,
Aug 19, 2014, 3:39:40 PM8/19/14
to Chris Bieneman, LLVM Dev
> I strongly feel this is the wrong decision. If you have a single process using two LLVM clients (say WebKit and Mesa), and they both are using an opt pass with different settings. If the storage is global this will not work.

That is only an issue if they can set the option at all.

To summarize, the point is that there are two *very* different types of options

* Nobs for which there is not a single right answer for all users.
There are very few of these currently and we expect it to remain like
that. These should not use cl::opt or static storage at all. They
should be an option passed to the constructor of the pass. It may
also involve exposing it to the C api and the PassManagerBuilder.

* Options that we use during development of llvm. They are useful for
testing, tracking a bug or enabling/disabling a feature that is still
under development. These can use a static storage since external
clients like webkit will never change them. We do have to avoid these
options requiring static constructors, since otherwise the client is
paying for something it will never use.

Chris Bieneman

unread,
Aug 19, 2014, 4:06:34 PM8/19/14
to Rafael Espíndola, LLVM Dev

> On Aug 19, 2014, at 12:32 PM, Rafael Espíndola <rafael.e...@gmail.com> wrote:
>
>> I strongly feel this is the wrong decision. If you have a single process using two LLVM clients (say WebKit and Mesa), and they both are using an opt pass with different settings. If the storage is global this will not work.
>
> That is only an issue if they can set the option at all.
>
> To summarize, the point is that there are two *very* different types of options
>
> * Nobs for which there is not a single right answer for all users.
> There are very few of these currently and we expect it to remain like
> that. These should not use cl::opt or static storage at all. They
> should be an option passed to the constructor of the pass. It may
> also involve exposing it to the C api and the PassManagerBuilder.

How would you suggest we expose cl::opts for modifying these options in tools like opt? A good example of this type of option would be the options in LoopUnrollPass.cpp.

>
> * Options that we use during development of llvm. They are useful for
> testing, tracking a bug or enabling/disabling a feature that is still
> under development. These can use a static storage since external
> clients like webkit will never change them. We do have to avoid these
> options requiring static constructors, since otherwise the client is
> paying for something it will never use.

What about when we're debugging the WebKit JIT? For development of libraries using LLVM it would be nice to be able to toggle these values too, which is why Filip’s suggestion of an API like LLVMConfigSetBoolValue(Config, "ScalarizeLoadStore", 1) would be nice.

-Chris

Rafael Espíndola

unread,
Aug 19, 2014, 4:35:20 PM8/19/14
to Chris Bieneman, LLVM Dev
>> * Nobs for which there is not a single right answer for all users.
>> There are very few of these currently and we expect it to remain like
>> that. These should not use cl::opt or static storage at all. They
>> should be an option passed to the constructor of the pass. It may
>> also involve exposing it to the C api and the PassManagerBuilder.
>
> How would you suggest we expose cl::opts for modifying these options in tools like opt? A good example of this type of option would be the options in LoopUnrollPass.cpp.

Opt uses the PassManagerBuilder. Opt itself could have a command line
options controlling its use of PassManagerBuilder. That is probably
fine since we expect very few of these.

>>
>> * Options that we use during development of llvm. They are useful for
>> testing, tracking a bug or enabling/disabling a feature that is still
>> under development. These can use a static storage since external
>> clients like webkit will never change them. We do have to avoid these
>> options requiring static constructors, since otherwise the client is
>> paying for something it will never use.
>
> What about when we're debugging the WebKit JIT? For development of libraries using LLVM it would be nice to be able to toggle these values too, which is why Filip’s suggestion of an API like LLVMConfigSetBoolValue(Config, "ScalarizeLoadStore", 1) would be nice.

Most llvm bugs reproduce with just opt or llc, but if that is not the
case, cl::ParseCommandLineOptions when debugging seems fine.

The advantages of this setup are

* Options that are exposed to the users are done so in a very natural
way (constructor arguments).
* Internal options are still really easy to create, but now don't have
static constructors.
* We don't need a LLVMConfig object that gets passed around.
* No stringly typed interface.

Chris Bieneman

unread,
Aug 19, 2014, 5:43:46 PM8/19/14
to Rafael Espíndola, LLVM Dev

> On Aug 19, 2014, at 1:32 PM, Rafael Espíndola <rafael.e...@gmail.com> wrote:
>
>>> * Nobs for which there is not a single right answer for all users.
>>> There are very few of these currently and we expect it to remain like
>>> that. These should not use cl::opt or static storage at all. They
>>> should be an option passed to the constructor of the pass. It may
>>> also involve exposing it to the C api and the PassManagerBuilder.
>>
>> How would you suggest we expose cl::opts for modifying these options in tools like opt? A good example of this type of option would be the options in LoopUnrollPass.cpp.
>
> Opt uses the PassManagerBuilder. Opt itself could have a command line
> options controlling its use of PassManagerBuilder. That is probably
> fine since we expect very few of these.
>
>>>
>>> * Options that we use during development of llvm. They are useful for
>>> testing, tracking a bug or enabling/disabling a feature that is still
>>> under development. These can use a static storage since external
>>> clients like webkit will never change them. We do have to avoid these
>>> options requiring static constructors, since otherwise the client is
>>> paying for something it will never use.
>>
>> What about when we're debugging the WebKit JIT? For development of libraries using LLVM it would be nice to be able to toggle these values too, which is why Filip’s suggestion of an API like LLVMConfigSetBoolValue(Config, "ScalarizeLoadStore", 1) would be nice.
>
> Most llvm bugs reproduce with just opt or llc, but if that is not the
> case, cl::ParseCommandLineOptions when debugging seems fine.

There are two reasons this doesn’t work:

(1) Cases where I might want to set a debug variable for the WebKit JIT but not for Mesa - if the option storage is global it will get overwritten for all users
(2) cl::ParseCommandLineOptions today is C++ API, WebKit restricts itself to the C API, so at the least we’d need to expose it as part of the C API

>
> The advantages of this setup are
>
> * Options that are exposed to the users are done so in a very natural
> way (constructor arguments).

I’m on board with this, but not to the exclusion of other use cases.

> * Internal options are still really easy to create, but now don't have
> static constructors.

We’re in agreement here

> * We don't need a LLVMConfig object that gets passed around.

For the sake of this discussion, let’s just scrap my phase two idea for a configuration object and treat that as a separate issue.

> * No stringly typed interface.

I think that there are advantages to a string-based interface. Sean Silva actually suggested that interface when I first sent out an email about our plans to rework command line options back on 8/6. Based on Sean’s feedback and a few discussions I had offline with other LLVM contributors I thought a stringly typed interface was the best approach to eliminating both the static constructors and the static storage which are explicit goals for our project.

Thanks,
-Chris

Rafael Espíndola

unread,
Aug 19, 2014, 6:13:04 PM8/19/14
to Chris Bieneman, LLVM Dev
> There are two reasons this doesn’t work:
>
> (1) Cases where I might want to set a debug variable for the WebKit JIT but not for Mesa - if the option storage is global it will get overwritten for all users

This really comes up? Really, we are not talking -O2/-O3 or inlining
thresholds. We are talking things like lcr-max-depth being needed for
a debugging session.

> (2) cl::ParseCommandLineOptions today is C++ API, WebKit restricts itself to the C API, so at the least we’d need to expose it as part of the C API

This seems a much smaller change than adding a LLVMConfig object.

>> * We don't need a LLVMConfig object that gets passed around.
>
> For the sake of this discussion, let’s just scrap my phase two idea for a configuration object and treat that as a separate issue.

But it makes a big difference on how the first phase is handled. If we
don't want the LLVMConfig object, the first phase should really just
remove the static constructors and not add a stringly typed interface.

> I think that there are advantages to a string-based interface. Sean Silva actually suggested that interface when I first sent out an email about our plans to rework command line options back on 8/6. Based on Sean’s feedback and a few discussions I had offline with other LLVM contributors I thought a stringly typed interface was the best approach to eliminating both the static constructors and the static storage which are explicit goals for our project.

Sorry I missed the original discussion. Sean, would you mind
summarizing the why of the stringly interface? Even if we do need a
LLVMConfig object (seems unlikely), I would still suggest using some
other key format. With strings we would suddenly be exposing every
command line option to the C API, which seems highly undesirable.

Chris Bieneman

unread,
Aug 19, 2014, 6:59:26 PM8/19/14
to Rafael Espíndola, LLVM Dev

> On Aug 19, 2014, at 3:10 PM, Rafael Espíndola <rafael.e...@gmail.com> wrote:
>
>> There are two reasons this doesn’t work:
>>
>> (1) Cases where I might want to set a debug variable for the WebKit JIT but not for Mesa - if the option storage is global it will get overwritten for all users
>
> This really comes up? Really, we are not talking -O2/-O3 or inlining
> thresholds. We are talking things like lcr-max-depth being needed for
> a debugging session.

I’ve toggled on SROA's "sroa-random-shuffle-slices” before for testing, I've played with InstCombine’s "enable-double-float-shrink”, so it does (although admittedly not terribly often).

>
>> (2) cl::ParseCommandLineOptions today is C++ API, WebKit restricts itself to the C API, so at the least we’d need to expose it as part of the C API
>
> This seems a much smaller change than adding a LLVMConfig object.
>
>>> * We don't need a LLVMConfig object that gets passed around.
>>
>> For the sake of this discussion, let’s just scrap my phase two idea for a configuration object and treat that as a separate issue.
>
> But it makes a big difference on how the first phase is handled. If we
> don't want the LLVMConfig object, the first phase should really just
> remove the static constructors and not add a stringly typed interface.

The stringly typed interface doesn’t just have to do with an LLVMConfig object, it also allows you to use non-static storage for options.

>
>> I think that there are advantages to a string-based interface. Sean Silva actually suggested that interface when I first sent out an email about our plans to rework command line options back on 8/6. Based on Sean’s feedback and a few discussions I had offline with other LLVM contributors I thought a stringly typed interface was the best approach to eliminating both the static constructors and the static storage which are explicit goals for our project.
>
> Sorry I missed the original discussion. Sean, would you mind
> summarizing the why of the stringly interface? Even if we do need a
> LLVMConfig object (seems unlikely), I would still suggest using some
> other key format. With strings we would suddenly be exposing every
> command line option to the C API, which seems highly undesirable.

The problem with alternate key formats from strings is it gets tricky when you start talking about supporting dynamically loaded passes and their options (which our current cl::opts do support).

Thanks,
-Chris

Chandler Carruth

unread,
Aug 19, 2014, 9:48:32 PM8/19/14
to Chris Bieneman, LLVM Dev
FWIW, I largely agree with Rafael's position here, at least in the near term.

The nice thing about it is that even if we don't stay there forever, it is a nice incremental improvement over the current state of the world, and we can actually be mindful going forward of whether the restriction it imposes (an inability to use "internal" knobs from within a library context that have multiple different library users in a single address space) proves to be a significant on-going burden.

On Tue, Aug 19, 2014 at 3:57 PM, Chris Bieneman <be...@apple.com> wrote:
> On Aug 19, 2014, at 3:10 PM, Rafael Espíndola <rafael.e...@gmail.com> wrote:
>
>> There are two reasons this doesn’t work:
>>
>> (1) Cases where I might want to set a debug variable for the WebKit JIT but not for Mesa - if the option storage is global it will get overwritten for all users
>
> This really comes up? Really, we are not talking -O2/-O3 or inlining
> thresholds. We are talking things like lcr-max-depth being needed for
> a debugging session.

I’ve toggled on SROA's "sroa-random-shuffle-slices” before for testing, I've played with InstCombine’s "enable-double-float-shrink”, so it does (although admittedly not terribly often).

I'm somewhat surprised that this comes up much in a context where you *can't* extract a test case and play with it using 'opt' or some other stand-alone context.

If these come up so rarely, would it be unreasonable to just flip the flag in the source code, and build a DSO to test with? For example, this is how I have done counter-based bisection and combine-based bisection of things (a similarly rare but necessary activity I suspect) and it seems to work well.

-Chandler

Sean Silva

unread,
Aug 20, 2014, 12:45:41 AM8/20/14
to Chris Bieneman, LLVM Dev
One interesting issue with moving away from the current system of static initializers for cl::opt is that we will no longer have the automatic registration of all the options so that -help will print everything available and generally we will not be able to issue an error for an "unknown command line option" (without calling into any other code).

The auto-registration is fundamentally tied with the globalness and the static initializers; pondering this has led me down an interesting path that has made me understand better my suggestion in the other thread. As I see it, there are two very different sorts of uses of llvm::cl in LLVM:

1. For regular command line processing. E.g. if a tool accepts an output file, then we need something that will parse the argument from the command line.

2. As a way to easily set up a conduit from A to B, where A is the command line and B is some place "deep" inside the LLVM library code that will do something in response to the command line.

(and, pending discussion, someday point A might include a proper programmatic interface (i.e. in a way other than hijacking the command line processing))

llvm::cl does a decent job for #1 and that is what it was designed for AFAICT; these uses of llvm::cl live outside of library code and everything is pretty happy, despite them being global and having static initializers.

The problem is that llvm::cl is not very well-suited to #2, yet it is used for #2, and that is the real problem. We need a solution to problem #2 which does not use llvm::cl. Thus, I don't think that the solution you propose here is the right direction.

The first step is to clearly differentiate between #1 and #2. I will say "command line options" for #1 and "configuration/tweak points" for #2. (maybe "library options" is better for #2; neither is perfect terminology)

The strawman I suggested in http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-August/075503.html was a stab at #2. There is no way to dodge being stringly typed since command lines are stringly typed, so really it is just a question of how long a solution stays stringly typed.

My thought process for staying stringly typed "the whole time" (possibly with some caching) comes from these two desires:
- adding a c/t point should require adding just one call into the c/t machinery (this is both for convenience and for DRY/SPOT), and 
- this change should be localized to the code being configured/tweaked
This is the thought process:

Note that llvm::cl is stringly typed until it parses the options. llvm::cl gives the appearance of a typed interface because it uses static initialization as a backdoor to globally transport the knowledge of the expected type to the option parsing machinery (very early in the program lifetime). Without this backdoor, we need to stay stringly typed longer, at least until we reach the "localized" place where the single call into the c/t machinery is made; this single call is the only place that has the type information needed for the c/t value to become properly typed. But there is no way to know how long it will be until we reach that point (or even *if* we reach that point; consider passes that are not run on this invocation).

Hence my suggestion of just putting a stringly typed key-value store (or whatever) in an easily accessible place (like LLVMContext), and just translating any unrecognized command line options (ones that are not for #1) into that stringly typed storage.

I agree with Rafael that "constructor arguments to passes" are not c/t points. In the future, there might be some way to integrate the two (from the referenced post, you can probably tell that I kind of like the idea of doing so), but for now, I think the clear incremental step is to attack #2 and solve it without llvm::cl. I have suggested a way to do this that I think makes sense.

-- Sean Silva






Sean Silva

unread,
Aug 20, 2014, 12:54:26 AM8/20/14
to Chris Bieneman, LLVM Dev
To be clear: I agree with Rafael that we need to tread very carefully about how we expose this machinery in the C API, if we expose it at all. My suggestion is completely orthogonal to this though; all I'm talking about is how to avoid the static constructors and global state caused by the cl::opt's in library code, which as I understand it is the motivation for the OP.

-- Sean Silva

Sean Silva

unread,
Aug 20, 2014, 1:08:14 AM8/20/14
to Rafael Espíndola, LLVM Dev
I think that what/how/if we expose in the C API is orthogonal to my suggestions, which are more about removing the static initializers and global state. The stringly typed interface is motivated by the need to late-resolve which options there are and what their types are; this need itself arises from eliminating the static initializers which serve as a backdoor for globally propagating type and option information on startup.

For more details, see my lengthy musings that I just replied to the OP.

Personally, I am very wary of exposing a stringly typed interface in the C API.

-- Sean Silva
 

Cheers,
Rafael

Pete Cooper

unread,
Aug 20, 2014, 1:12:13 AM8/20/14
to Chandler Carruth, LLVM Dev
On Aug 19, 2014, at 6:45 PM, Chandler Carruth <chan...@google.com> wrote:

FWIW, I largely agree with Rafael's position here, at least in the near term.

The nice thing about it is that even if we don't stay there forever, it is a nice incremental improvement over the current state of the world, and we can actually be mindful going forward of whether the restriction it imposes (an inability to use "internal" knobs from within a library context that have multiple different library users in a single address space) proves to be a significant on-going burden.
I actually disagree with this for two reasons.

The first is that if there are going to be changes to the code anyway to remove static initializers, and we can move the storage to the pass at the same time, the we should make just one change and not two.

The second reason is that in most cases these knobs should not be globals. If I had a piece of data (not a CL::opt) in global scope, only used by one pass, then I'm sure people would question why it's a global at all and move it inside the class. We're treating cl::opt as special here when there's no reason for the opt or the storage to be global. 

We frown upon the use of globals, otherwise LLVM would be littered with them like many other C++ code bases. I don't think cl::opts should be special at all in this respect.

Thanks
Pete


On Tue, Aug 19, 2014 at 3:57 PM, Chris Bieneman <be...@apple.com> wrote:
> On Aug 19, 2014, at 3:10 PM, Rafael Espíndola <rafael.e...@gmail.com> wrote:
>
>> There are two reasons this doesn’t work:
>>
>> (1) Cases where I might want to set a debug variable for the WebKit JIT but not for Mesa - if the option storage is global it will get overwritten for all users
>
> This really comes up? Really, we are not talking -O2/-O3 or inlining
> thresholds. We are talking things like lcr-max-depth being needed for
> a debugging session.

I’ve toggled on SROA's "sroa-random-shuffle-slices” before for testing, I've played with InstCombine’s "enable-double-float-shrink”, so it does (although admittedly not terribly often).

I'm somewhat surprised that this comes up much in a context where you *can't* extract a test case and play with it using 'opt' or some other stand-alone context.

If these come up so rarely, would it be unreasonable to just flip the flag in the source code, and build a DSO to test with? For example, this is how I have done counter-based bisection and combine-based bisection of things (a similarly rare but necessary activity I suspect) and it seems to work well.

-Chandler
_______________________________________________

Chandler Carruth

unread,
Aug 20, 2014, 1:27:01 AM8/20/14
to Pete Cooper, LLVM Dev
On Tue, Aug 19, 2014 at 10:10 PM, Pete Cooper <peter_...@apple.com> wrote:
On Aug 19, 2014, at 6:45 PM, Chandler Carruth <chan...@google.com> wrote:

FWIW, I largely agree with Rafael's position here, at least in the near term.

The nice thing about it is that even if we don't stay there forever, it is a nice incremental improvement over the current state of the world, and we can actually be mindful going forward of whether the restriction it imposes (an inability to use "internal" knobs from within a library context that have multiple different library users in a single address space) proves to be a significant on-going burden.
I actually disagree with this for two reasons.

The first is that if there are going to be changes to the code anyway to remove static initializers, and we can move the storage to the pass at the same time, the we should make just one change and not two.

No one is suggesting otherwise that I have seen? At least, my interpretation (perhaps incorrect, I've not had time to read all of this thread in 100% detail) was that the removal of static initializers wouldn't require changing every cl::opt variable.
 

The second reason is that in most cases these knobs should not be globals. If I had a piece of data (not a CL::opt) in global scope, only used by one pass, then I'm sure people would question why it's a global at all and move it inside the class. We're treating cl::opt as special here when there's no reason for the opt or the storage to be global. 

We frown upon the use of globals, otherwise LLVM would be littered with them like many other C++ code bases. I don't think cl::opts should be special at all in this respect.

Sure, you're arguing against the core design of cl::opt. However, we have it, and it wasn't an accident or for lack of other patterns that we chose it.

For example, we don't require all constants to be per-pass, and instead have per-file constants. Rafael is suggesting that one use case for cl::opt global variables is, in essence, a constant that is somewhat easier for a developer of LLVM (*not* a user) to change during debugging and active development. I don't think the desire for convenience and only supporting the default value in production contexts are completely invalid.

Once you factor those in, we have a tradeoff. Historically, the tradeoff was made in the direction of convenience, relying on a very narrow interpretation of the use cases. It isn't clear to me that we should, today, make a different tradeoff. It certainly doesn't make sense why you would gate removing static initializers (a clear win) on forcing a change on a core design pattern within LLVM which not all of the developers are really supportive of (at this point).

Renato Golin

unread,
Aug 20, 2014, 1:31:43 AM8/20/14
to Sean Silva, LLVM Dev
On 20 August 2014 05:43, Sean Silva <chiso...@gmail.com> wrote:
> 2. As a way to easily set up a conduit from A to B, where A is the command
> line and B is some place "deep" inside the LLVM library code that will do
> something in response to the command line.

I never liked this as a solution to #2. Heck, I never liked that we do
have #2 in the first place.


> Hence my suggestion of just putting a stringly typed key-value store (or
> whatever) in an easily accessible place (like LLVMContext), and just
> translating any unrecognized command line options (ones that are not for #1)
> into that stringly typed storage.

I fully support this idea, and is in line with my strawman proposal
for FPU/CPU parsing on all tools shared (beyond the boundaries of
LLVM).

String parsing is common and, even being target specific or pass
specific, is still string parsing and should be identical to all users
of the *same* feature.

I also believe a more sane command line option scheme is in order.
Today we have a zillion of options that are completely disconnected,
documented by accident in the initializer, without any bigger context
whatsoever. This is in part to follow what GCC has always done, and
probably we'll still need to support GCC's and our own legacy for
decades, but all that can also live in this commoned up parser.

Specifically to #2, my idea was something like:
--vectorizer-opts="foo,bar,baz=10" --tbaa-opts="...", etc. That could
use a common parser all the way down to parsing "foo", which would be
left by the vectorizer's back-end to the parser to deal with and setup
the right flags in the right structure, used because "vectorizer" in
"vectorizer-opts" tell the factory to return a vectorizer parser's
back-end.

The FPU/CPU parsing (which has to parse command line options and
assembly directives, which happens to have the same syntax), would
have a similar structure.

Such flags in LLVMContext (or whatever) would have to be structured
like a tree and each pass should receive its own tree root, which most
of the time would have just a list of key/values, but some times have
a more nested structure.

cheers,
--renato

Pete Cooper

unread,
Aug 20, 2014, 2:01:54 AM8/20/14
to Sean Silva, LLVM Dev
I have no problem with this either.

As an alternative, and one which is likely to be fairly stable, we could just expose ParseCommandLineOptions to the C API.  That parsing will cause the options to change anyway, and there’s actually nothing to stop you from calling it multiple times to change different options at different times.

Pete

-- Sean Silva
 

Cheers,
Rafael

Pete Cooper

unread,
Aug 20, 2014, 2:14:24 AM8/20/14
to Sean Silva, LLVM Dev
On Aug 19, 2014, at 9:43 PM, Sean Silva <chiso...@gmail.com> wrote:

One interesting issue with moving away from the current system of static initializers for cl::opt is that we will no longer have the automatic registration of all the options so that -help will print everything available and generally we will not be able to issue an error for an "unknown command line option" (without calling into any other code).
Not automatic no, but in the proposal Chris puts the addOption call inside the pass initializer which is called before ParseCommandLineOptions.  This means you’ll still get options listed as you currently do, so long as you continue to calls the pass initializers before parse (something you have to do anyway to get the pass name visible to the command line)


The auto-registration is fundamentally tied with the globalness and the static initializers; pondering this has led me down an interesting path that has made me understand better my suggestion in the other thread. As I see it, there are two very different sorts of uses of llvm::cl in LLVM:

1. For regular command line processing. E.g. if a tool accepts an output file, then we need something that will parse the argument from the command line.

2. As a way to easily set up a conduit from A to B, where A is the command line and B is some place "deep" inside the LLVM library code that will do something in response to the command line.

(and, pending discussion, someday point A might include a proper programmatic interface (i.e. in a way other than hijacking the command line processing))
That would be nice.  I just suggested in another thread that we expose ParseCommandLineOptions to the C API to hack around this, but a nice clean interface would of course be better.


llvm::cl does a decent job for #1 and that is what it was designed for AFAICT; these uses of llvm::cl live outside of library code and everything is pretty happy, despite them being global and having static initializers.

The problem is that llvm::cl is not very well-suited to #2, yet it is used for #2, and that is the real problem. We need a solution to problem #2 which does not use llvm::cl. Thus, I don't think that the solution you propose here is the right direction.

The first step is to clearly differentiate between #1 and #2. I will say "command line options" for #1 and "configuration/tweak points" for #2. (maybe "library options" is better for #2; neither is perfect terminology)

The strawman I suggested in http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-August/075503.html was a stab at #2. There is no way to dodge being stringly typed since command lines are stringly typed, so really it is just a question of how long a solution stays stringly typed.

My thought process for staying stringly typed "the whole time" (possibly with some caching) comes from these two desires:
- adding a c/t point should require adding just one call into the c/t machinery (this is both for convenience and for DRY/SPOT), and 
Right.  The current point is in the pass initializer.

There is another point in the pass constructor to read the option value.  This is the only point at which something will change from being a string to its actual type and value.

- this change should be localized to the code being configured/tweaked
This is the thought process:

Note that llvm::cl is stringly typed until it parses the options. llvm::cl gives the appearance of a typed interface because it uses static initialization as a backdoor to globally transport the knowledge of the expected type to the option parsing machinery (very early in the program lifetime). Without this backdoor, we need to stay stringly typed longer, at least until we reach the "localized" place where the single call into the c/t machinery is made; this single call is the only place that has the type information needed for the c/t value to become properly typed. But there is no way to know how long it will be until we reach that point (or even *if* we reach that point; consider passes that are not run on this invocation).
The current proposal exposes the type in addOption (as well as later when we get the option). So the type continues to be known to the command line parser.  Whether you want to actually type check in the command line is a point i’m open to discuss.  Personally i want a command line option to be type checked because it was registered, even if no-one actually gets the value of the option later.


Hence my suggestion of just putting a stringly typed key-value store (or whatever) in an easily accessible place (like LLVMContext), and just translating any unrecognized command line options (ones that are not for #1) into that stringly typed storage.
I’m against it being in the context because you may want to set up and reuse passes multiple times with the same options, and use that configuration to compile multiple LLVMContexts.  But I do agree that having a store with some lifetime is useful.

I think the current proposal is to have the store be a singleton, but there’s nothing to stop further work to have the storage for options be one per thread for example.  If you wanted to have one pass manager per thread with its own set of passes, configured (currently) via their own call to ParseCommandLineOptions then that would be possible with little work beyond the current proposal.


I agree with Rafael that "constructor arguments to passes" are not c/t points. In the future, there might be some way to integrate the two (from the referenced post, you can probably tell that I kind of like the idea of doing so), but for now, I think the clear incremental step is to attack #2 and solve it without llvm::cl. I have suggested a way to do this that I think makes sense.
If you change the current proposal so that it doesn’t read cl::opt, then I think this reads to me like what is being proposed now.  Really its creating a string->string map with addOption, and getting the values with getOption.  The passes don’t care (or know) whether the options are set via the command line or any other API.  I hope i’ve understood your proposal correctly here.  Please correct me otherwise.

Thanks,
Pete

Chris Bieneman

unread,
Aug 20, 2014, 2:22:37 AM8/20/14
to Chandler Carruth, LLVM Dev
On Aug 19, 2014, at 10:24 PM, Chandler Carruth <chan...@google.com> wrote:


On Tue, Aug 19, 2014 at 10:10 PM, Pete Cooper <peter_...@apple.com> wrote:
On Aug 19, 2014, at 6:45 PM, Chandler Carruth <chan...@google.com> wrote:

FWIW, I largely agree with Rafael's position here, at least in the near term.

The nice thing about it is that even if we don't stay there forever, it is a nice incremental improvement over the current state of the world, and we can actually be mindful going forward of whether the restriction it imposes (an inability to use "internal" knobs from within a library context that have multiple different library users in a single address space) proves to be a significant on-going burden.
I actually disagree with this for two reasons.

The first is that if there are going to be changes to the code anyway to remove static initializers, and we can move the storage to the pass at the same time, the we should make just one change and not two.

No one is suggesting otherwise that I have seen? At least, my interpretation (perhaps incorrect, I've not had time to read all of this thread in 100% detail) was that the removal of static initializers wouldn't require changing every cl::opt variable.

I think I may be misunderstanding you here. If I understand Rafael correctly he wants all the option storage to be done using the cl::opt external storage capabilities, so that the option values are stored globally.

My original proposal was to leave the storage in cl::opt, but to move the cl::opt to being owned, and have a keyed lookup. My plan was geared around being able to change one cl::opt at a time, but ultimately to get rid of the static initializers you do need to change every cl::opt variable so that they aren’t global.

As part of this work I did want to eliminate the global storage for all these options in favor of having the data stored in the passes. It seems that this last is the contentious part, which is what Pete is talking about WRT the use of globals.

I’m also not sure how Rafael’s proposal will work with eliminating static initializers for cl::opts with class or list typed data.

 

The second reason is that in most cases these knobs should not be globals. If I had a piece of data (not a CL::opt) in global scope, only used by one pass, then I'm sure people would question why it's a global at all and move it inside the class. We're treating cl::opt as special here when there's no reason for the opt or the storage to be global. 

We frown upon the use of globals, otherwise LLVM would be littered with them like many other C++ code bases. I don't think cl::opts should be special at all in this respect.

Sure, you're arguing against the core design of cl::opt. However, we have it, and it wasn't an accident or for lack of other patterns that we chose it.

For example, we don't require all constants to be per-pass, and instead have per-file constants. Rafael is suggesting that one use case for cl::opt global variables is, in essence, a constant that is somewhat easier for a developer of LLVM (*not* a user) to change during debugging and active development. I don't think the desire for convenience and only supporting the default value in production contexts are completely invalid.

I don’t think that my proposal adversely impacts the ability for a developer of LLVM to change the value of an option during debugging and development. If anything it makes this easier because it provides a way to do so without modifying source code (while not preventing you from doing it by modifying source code).


Once you factor those in, we have a tradeoff. Historically, the tradeoff was made in the direction of convenience, relying on a very narrow interpretation of the use cases. It isn't clear to me that we should, today, make a different tradeoff. It certainly doesn't make sense why you would gate removing static initializers (a clear win) on forcing a change on a core design pattern within LLVM which not all of the developers are really supportive of (at this point).

I guess my point here is that there doesn’t need to be a tradeoff. My proposal doesn’t adversely impact convenience, and supports a wider use case.

I do agree that we shouldn’t gate removing the static initializers on removing the globals, but I also don’t think this is really forcing a core design pattern change. If we can’t come to an agreement on the global data we can shelve the conversation.

-Chris

Pete Cooper

unread,
Aug 20, 2014, 2:25:26 AM8/20/14
to Chandler Carruth, LLVM Dev
On Aug 19, 2014, at 10:24 PM, Chandler Carruth <chan...@google.com> wrote:


On Tue, Aug 19, 2014 at 10:10 PM, Pete Cooper <peter_...@apple.com> wrote:
On Aug 19, 2014, at 6:45 PM, Chandler Carruth <chan...@google.com> wrote:

FWIW, I largely agree with Rafael's position here, at least in the near term.

The nice thing about it is that even if we don't stay there forever, it is a nice incremental improvement over the current state of the world, and we can actually be mindful going forward of whether the restriction it imposes (an inability to use "internal" knobs from within a library context that have multiple different library users in a single address space) proves to be a significant on-going burden.
I actually disagree with this for two reasons.

The first is that if there are going to be changes to the code anyway to remove static initializers, and we can move the storage to the pass at the same time, the we should make just one change and not two.

No one is suggesting otherwise that I have seen? At least, my interpretation (perhaps incorrect, I've not had time to read all of this thread in 100% detail) was that the removal of static initializers wouldn't require changing every cl::opt variable.
It won’t no.  The majority of static initializers are globals in passes, but cl::opt’s and other globals in tools for example are out of scope for this work right now (there’s no opt.cpp in a dylib so we don’t care about its globals for example).

 

The second reason is that in most cases these knobs should not be globals. If I had a piece of data (not a CL::opt) in global scope, only used by one pass, then I'm sure people would question why it's a global at all and move it inside the class. We're treating cl::opt as special here when there's no reason for the opt or the storage to be global. 

We frown upon the use of globals, otherwise LLVM would be littered with them like many other C++ code bases. I don't think cl::opts should be special at all in this respect.

Sure, you're arguing against the core design of cl::opt. However, we have it, and it wasn't an accident or for lack of other patterns that we chose it.
Oh yeah, its a fine solution for a tricky problem, but now that we’re having this discussion its clear that its use of static initializers is itself a problem.


For example, we don't require all constants to be per-pass, and instead have per-file constants. Rafael is suggesting that one use case for cl::opt global variables is, in essence, a constant that is somewhat easier for a developer of LLVM (*not* a user) to change during debugging and active development. I don't think the desire for convenience and only supporting the default value in production contexts are completely invalid.
I can see what you’re saying here, but i’m not convinced that changing the value of a constant in global scope is any easier than changing its value in the pass constructor.


Once you factor those in, we have a tradeoff. Historically, the tradeoff was made in the direction of convenience, relying on a very narrow interpretation of the use cases. It isn't clear to me that we should, today, make a different tradeoff. It certainly doesn't make sense why you would gate removing static initializers (a clear win) on forcing a change on a core design pattern within LLVM which not all of the developers are really supportive of (at this point).
I agree here.  There’s not a strict need to move the option storage for something like an unsigned in to a pass using it (there may be for a std::string for which we’d again have a static initializer).  However, I do think its the right thing to do in terms of coding guidelines.  If the code to get and set an option is already in the pass initializer/constructor respectively, then I don’t think the storage should have to live outside the pass just to minimize a patch.

Now saying this, I don’t think if the community agreed to this proposal as is, that it would mean blanket approval to change all cl::opts in all cases.  But I think where its an easy change, and obviously the right choice, that options as well as their storage should be allowed to be moved in to the pass.  If it makes sense to move only the option but not the storage then that should be done as a first step, and a discussion started on the right thing for the storage.

Thanks,
Pete

Rafael Espíndola

unread,
Aug 20, 2014, 11:54:14 AM8/20/14
to Pete Cooper, LLVM Dev
> FWIW, I largely agree with Rafael's position here, at least in the near
> term.
>
> The nice thing about it is that even if we don't stay there forever, it is a
> nice incremental improvement over the current state of the world, and we can
> actually be mindful going forward of whether the restriction it imposes (an
> inability to use "internal" knobs from within a library context that have
> multiple different library users in a single address space) proves to be a
> significant on-going burden.
>
> I actually disagree with this for two reasons.
>
> The first is that if there are going to be changes to the code anyway to
> remove static initializers, and we can move the storage to the pass at the
> same time, the we should make just one change and not two.
>
> The second reason is that in most cases these knobs should not be globals.
> If I had a piece of data (not a CL::opt) in global scope, only used by one
> pass, then I'm sure people would question why it's a global at all and move
> it inside the class. We're treating cl::opt as special here when there's no
> reason for the opt or the storage to be global.
>
> We frown upon the use of globals, otherwise LLVM would be littered with them
> like many other C++ code bases. I don't think cl::opts should be special at
> all in this respect.


Note that the call

cl::OptionRegistry::CreateOption<bool>(&ScalarizeLoadStore,
"ScalarizeLoadStore",
"scalarize-load-store", cl::Hidden, cl::init(false),
cl::desc("Allow the scalarizer pass to scalarize loads and store"));

ScalarizeLoadStore can actually be a member variable as long as caller
guarantees it is still around when the command line is parsed. I have
no objections to doing this move in the first pass if you want to.

What I would really like to avoid for now is the

cl::OptionRegistry::GetValue<bool>("ScalarizeLoadStore");

Cheers,
Rafael

Pete Cooper

unread,
Aug 20, 2014, 12:18:06 PM8/20/14