llvm dev - Aug 2014 - [LLVMdev] [RFC] Removing static initializers for command line options

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
> On Aug 19, 2014, at 8:29 AM, Filip Pizlo <fpizlo at apple.com> wrote:
> 
> 
> 
> On Aug 18, 2014, at 4:30 PM, Chris Bieneman <beanz at apple.com
<mailto:beanz at apple.com>> wrote:
> 
>> 
>>> On Aug 18, 2014, at 4:25 PM, Filip Pizlo <fpizlo at apple.com
<mailto:fpizlo at apple.com>> wrote:
>>> 
>>> 
>>> 
>>>> On Aug 18, 2014, at 3:21 PM, Chris Bieneman <beanz at
apple.com <mailto:beanz at apple.com>> wrote:
>>>> 
>>>> 
>>>>>> On Aug 18, 2014, at 3:09 PM, Rafael Espíndola
<rafael.espindola at gmail.com <mailto:rafael.espindola at
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.
>>> 
>>> 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
> 
>> 
>> -Chris
>> 
>>> 
>>>> 
>>>> I do agree that we should ultimately drop the cl namespace if
we’re going in this direction.
>>>> 
>>>> -Chris
>>>> 
>>>>> 
>>>>> Cheers,
>>>>> Rafael
>>>> 
>>>> 
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> LLVMdev at cs.uiuc.edu <mailto:LLVMdev at cs.uiuc.edu>   
http://llvm.cs.uiuc.edu <http://llvm.cs.uiuc.edu/>
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
<http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140819/181f84d9/attachment.html>

On 19 August 2014 13:47, Chris Bieneman <beanz at 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.

Cheers,
Rafael

> On Aug 19, 2014, at 11:09 AM, Rafael Espíndola <rafael.espindola at
gmail.com> wrote:
> 
>> On 19 August 2014 13:47, Chris Bieneman <beanz at 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
> 
> 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

> On Aug 19, 2014, at 11:09 AM, Rafael Espíndola <rafael.espindola at
gmail.com> wrote:
> 
> On 19 August 2014 13:47, Chris Bieneman <beanz at 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
> 
>> Does this sound reasonable for a first step?
> 
> Removing the static constructors does.
> 
> Cheers,
> Rafael