[RFC] Removing static initializers for command line options

Today command line arguments in LLVM are global variables. An example argument from Scalarizer.cpp is:

static cl::opt 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(“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(“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 (12 KB)

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");

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.

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.

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

Today command line arguments in LLVM are global variables. An example
argument from Scalarizer.cpp is:

static cl::opt 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(“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(“ScalarizeLoadStore”);

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(&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.

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.

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.

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
(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

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 :slight_smile:

Cheers,
Rafael

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 :slight_smile:

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

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 :slight_smile:

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

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.

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 :slight_smile:

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

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.

How do you construct you PassManager?

-Chris

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.

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 :slight_smile:

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

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.

How do you construct you PassManager?

LLVMCreatePassManager() and then we add target data, add a target machine analysis pass, and then we construct our pipeline and call LLVMRunPassManager().

-Filip

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

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 :frowning:

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.

Cheers,
Rafael

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 :frowning:

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

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.

Cheers,
Rafael

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 :frowning:

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

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.

Cheers,
Rafael

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

* 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.

Cheers,
Rafael

* 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

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.

Cheers,
Rafael

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