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

> On Aug 19, 2014, at 1:32 PM, Rafael Espíndola <rafael.espindola at
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
> 
> 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.
> (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

> On Aug 19, 2014, at 3:10 PM, Rafael Espíndola <rafael.espindola at
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
> 
> Cheers,
> Rafael

On Tue, Aug 19, 2014 at 3:10 PM, Rafael Espíndola <
rafael.espindola at 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.
>
> > (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.
>
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140819/986c2e8b/attachment.html>

> On Aug 19, 2014, at 6:45 PM, Chandler Carruth <chandlerc at
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 <beanz at
apple.com> wrote:
>> > On Aug 19, 2014, at 3:10 PM, Rafael Espíndola <rafael.espindola
at 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
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> 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/e5b98266/attachment.html>

On Tue, Aug 19, 2014 at 10:10 PM, Pete Cooper <peter_cooper at apple.com>
wrote:
> On Aug 19, 2014, at 6:45 PM, Chandler Carruth <chandlerc at
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).
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140819/daa8dc07/attachment.html>

> On Aug 19, 2014, at 10:24 PM, Chandler Carruth <chandlerc at
google.com> wrote:
> 
> 
> On Tue, Aug 19, 2014 at 10:10 PM, Pete Cooper <peter_cooper at apple.com
<mailto:peter_cooper at apple.com>> wrote:
>> On Aug 19, 2014, at 6:45 PM, Chandler Carruth <chandlerc at
google.com <mailto:chandlerc at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140819/c29caae3/attachment.html>

> On Aug 19, 2014, at 10:24 PM, Chandler Carruth <chandlerc at
google.com> wrote:
> 
> 
> On Tue, Aug 19, 2014 at 10:10 PM, Pete Cooper <peter_cooper at
apple.com> wrote:
>> On Aug 19, 2014, at 6:45 PM, Chandler Carruth <chandlerc at
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

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140819/d25e9982/attachment.html>

> 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

> On Aug 20, 2014, at 8:51 AM, Rafael Espíndola <rafael.espindola at
gmail.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.
> 
> 
> 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”);
Ah, I totally see what you mean now.  Sorry if i had been confused before.

To be honest, I had exactly the same reservations myself, but then i looked in
to how we initialize and create passes and there actually isn’t a nice way to do
this right now.

The trouble is that INITIALIZE_PASS doesn’t actually construct a pass.  It
actually constructs a PassInfo which has a pointer to the default constructor of
the pass.  Later, if the pass manager needs to (say -scalarize was on the
commandline), it will get the default constructor from PassInfo and construct
the pass.

Unfortunately, this completely detaches the lifetime of the pass instance where
we want to store the ScalarizeLoadStore bool, from the option code to hook in to
the cl::opt stuff.  I originally wanted to do something gross like pass
__offset_of(Scalarizer::ScalarizeLoadStore) to the PassInfo and hide the
initialization of the option in there.  But that doesn’t work either, as there’s
no guarantee you’ll even create a instance of Scalarizer, even though you could
pass -scalarize-load-store to the command line.

So, we do have 2 solutions: a global variable, and a call to GetValue.  As you
can see, either way isn’t perfect, but if you can think of another solution i’m
sure we can discuss it.

Thanks,
Pete
> 
> Cheers,
> Rafael

> On Aug 20, 2014, at 9:15 AM, Pete Cooper <peter_cooper at apple.com>
wrote:
> 
> 
>> On Aug 20, 2014, at 8:51 AM, Rafael Espíndola <rafael.espindola at
gmail.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.
>> 
>> 
>> 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”);
> Ah, I totally see what you mean now.  Sorry if i had been confused before.
> 
> To be honest, I had exactly the same reservations myself, but then i looked
in to how we initialize and create passes and there actually isn’t a nice way to
do this right now.
> 
> The trouble is that INITIALIZE_PASS doesn’t actually construct a pass.  It
actually constructs a PassInfo which has a pointer to the default constructor of
the pass.  Later, if the pass manager needs to (say -scalarize was on the
commandline), it will get the default constructor from PassInfo and construct
the pass.
> 
> Unfortunately, this completely detaches the lifetime of the pass instance
where we want to store the ScalarizeLoadStore bool, from the option code to hook
in to the cl::opt stuff.  I originally wanted to do something gross like pass
__offset_of(Scalarizer::ScalarizeLoadStore) to the PassInfo and hide the
initialization of the option in there.  But that doesn’t work either, as there’s
no guarantee you’ll even create a instance of Scalarizer, even though you could
pass -scalarize-load-store to the command line.
> 
> So, we do have 2 solutions: a global variable, and a call to GetValue.  As
you can see, either way isn’t perfect, but if you can think of another solution
i’m sure we can discuss it.
> 
This is very raw, so excuse any mistakes in the code, but I think i came up with
a third option.

What if we added a static method to the Scalarizer class.  This method takes
pointers to each option storage.  If a pointer is null, the function is being
called from INITIALIZE_PASS and so we create all the options.  If a pointer is
not null, we’re being called from the pass constructor and we set the value of
that option.  I think it would look something like this (which can of course be
tidied up).

  static void addOptions(bool *ScalarizeLoadStore = nullptr) {
    Option::iterator ScalarizeLoadStoreOpt      
getOrAddOption<bool>("scalarize-load-store");
    if (ScalarizeLoadStore) {
      // Get the value of the option.
      *ScalarizeLoadStore = ScalarizeLoadStoreOpt.getValue();
    } else {
      // Adding the option with this name.  Set up its properties.
      ScalarizeLoadStoreOpt.init(cl::Hidden, cl::init(false),
                                 cl::desc("Allow the scalarizer pass to
scalarize loads and store"));
    }
  }

Pete
> Thanks,
> Pete
>> 
>> Cheers,
>> Rafael
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

> So, we do have 2 solutions: a global variable, and a call to GetValue.  As
you can see, either way isn’t perfect, but if you can think of another solution
i’m sure we can discuss it.
Pass the passinfo to the pass constructor maybe?

I still don't understand what the problem with the global is. It has
the same value for all users. As Chandler pointed out, having

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

right now is just our way of making

// Allow the scalarizer pass to scalarize loads and store
const static bool ScalarizeLoadStore = false;

more convenient to developers so they don't have to edit/compile to
test it with a different value. From a library user point of view they
should be exactly the same: a constant that is *always* false.

Another option (not my preference) would be to use globals just as keys:

typedef <something> LLVMOptionKey;
...
static LLVMOptionKey ScalarizeLoadStoreKey; //global
...
cl::OptionRegistry::createOption<bool>(&ScalarizeLoadStoreKey,
"ScalarizeLoadStore",
   "scalarize-load-store", cl::Hidden, cl::init(false),
   cl::desc("Allow the scalarizer pass to scalarize loads and
store"));
....
bool ScalarizeLoadStore
cl::OptionRegistry::getValue<bool>(&ScalarizeLoadStoreKey); // local

That way we avoid exposing ScalarizeLoadStore to library users since
the getValue takes a key they cannot guess instead of well known
string.

Cheers,
Rafael

> This is very raw, so excuse any mistakes in the code, but I think i came up
with a third option.
>
> What if we added a static method to the Scalarizer class.  This method
takes pointers to each option storage.  If a pointer is null, the function is
being called from INITIALIZE_PASS and so we create all the options.  If a
pointer is not null, we’re being called from the pass constructor and we set the
value of that option.  I think it would look something like this (which can of
course be tidied up).
>
>   static void addOptions(bool *ScalarizeLoadStore = nullptr) {
>     Option::iterator ScalarizeLoadStoreOpt >      
getOrAddOption<bool>("scalarize-load-store");
>     if (ScalarizeLoadStore) {
>       // Get the value of the option.
>       *ScalarizeLoadStore = ScalarizeLoadStoreOpt.getValue();
>     } else {
>       // Adding the option with this name.  Set up its properties.
>       ScalarizeLoadStoreOpt.init(cl::Hidden, cl::init(false),
>                                  cl::desc("Allow the scalarizer pass
to scalarize loads and store"));
>     }
>   }
As long as we are careful to make sure only the command line parsing
can set this, it should be fine. It has some nice properties missing
from the original proposal (including stage2)

* "scalarize-load-store" is written once. Not extra string keys.
* It seems easier to hide it.
* We don't need a LLVMConfig object that gets passed around.
* There is exactly one of exposing features to libraries: change the
constructor and maybe the PassManagerBuilder.

I still don't see the advantage over a static storage, but I am not
strongly oppose to it, as long as there is nothing like
cl::OptionRegistry::SetValue<bool>("ScalarizeLoadStore", true)
that
libraries can use.

Cheers,
Rafael

> On Aug 20, 2014, at 9:31 AM, Rafael Espíndola <rafael.espindola at
gmail.com> wrote:
> 
>> So, we do have 2 solutions: a global variable, and a call to GetValue. 
As you can see, either way isn’t perfect, but if you can think of another
solution i’m sure we can discuss it.
> 
> Pass the passinfo to the pass constructor maybe?
> 
> I still don't understand what the problem with the global is. It has
> the same value for all users. As Chandler pointed out, having
I just assumed that all globals are bad.  Perhaps thats not true in all
cases.
> 
> static cl::opt<bool> ScalarizeLoadStore
>  ("scalarize-load-store", cl::Hidden, cl::init(false),
>   cl::desc("Allow the scalarizer pass to scalarize loads and
store"));
> 
> right now is just our way of making
> 
> // Allow the scalarizer pass to scalarize loads and store
> const static bool ScalarizeLoadStore = false;
> 
> more convenient to developers so they don't have to edit/compile to
> test it with a different value. From a library user point of view they
> should be exactly the same: a constant that is *always* false.
> 
> Another option (not my preference) would be to use globals just as keys:
> 
> typedef <something> LLVMOptionKey;
> ...
> static LLVMOptionKey ScalarizeLoadStoreKey; //global
You could do this if the key is the name of the option, but that detaches the
name from the rest of the calls which may be quite far apart in the code. 
Otherwise so long as the key type doesn’t have a static constructor, this is
fine with me.

Thanks,
Pete
> ...
> cl::OptionRegistry::createOption<bool>(&ScalarizeLoadStoreKey,
> "ScalarizeLoadStore",
>   "scalarize-load-store", cl::Hidden, cl::init(false),
>   cl::desc("Allow the scalarizer pass to scalarize loads and
store"));
> ....
> bool ScalarizeLoadStore >
cl::OptionRegistry::getValue<bool>(&ScalarizeLoadStoreKey); // local
> 
> That way we avoid exposing ScalarizeLoadStore to library users since
> the getValue takes a key they cannot guess instead of well known
> string.
> 
> Cheers,
> Rafael

>> Another option (not my preference) would be to use globals just as
keys:
>>
>> typedef <something> LLVMOptionKey;
>> ...
>> static LLVMOptionKey ScalarizeLoadStoreKey; //global
> You could do this if the key is the name of the option, but that detaches
the name from the rest of the calls which may be quite far apart in the code. 
Otherwise so long as the key type doesn’t have a static constructor, this is
fine with me.
The Key global is only used for its unique address, so LLVMOptionKey
could be just "void *" for example.

Cheers,
Rafael

> On Aug 20, 2014, at 9:31 AM, Rafael Espíndola <rafael.espindola at
gmail.com> wrote:
> 
>> So, we do have 2 solutions: a global variable, and a call to GetValue. 
As you can see, either way isn’t perfect, but if you can think of another
solution i’m sure we can discuss it.
> 
> Pass the passinfo to the pass constructor maybe?
> 
> I still don't understand what the problem with the global is. It has
> the same value for all users. As Chandler pointed out, having
Globals are bad for many reasons: namespace pollution, concurrency issues, lack
of access control, etc. etc.. Some of them have been discussed in this thread.
Perhaps it’s not a big concern for most of the LLVM users. But we have an unique
environment where LLVM is shared by multiple clients, and where the concern
around exploits are especially strong. So while eliminating globals is not
strictly tied to the elimination of static initializers, it is still a strong
goal towards providing a LLVM dylib.

Evan
> 
> static cl::opt<bool> ScalarizeLoadStore
>  ("scalarize-load-store", cl::Hidden, cl::init(false),
>   cl::desc("Allow the scalarizer pass to scalarize loads and
store"));
> 
> right now is just our way of making
> 
> // Allow the scalarizer pass to scalarize loads and store
> const static bool ScalarizeLoadStore = false;
> 
> more convenient to developers so they don't have to edit/compile to
> test it with a different value. From a library user point of view they
> should be exactly the same: a constant that is *always* false.
> 
> Another option (not my preference) would be to use globals just as keys:
> 
> typedef <something> LLVMOptionKey;
> ...
> static LLVMOptionKey ScalarizeLoadStoreKey; //global
> ...
> cl::OptionRegistry::createOption<bool>(&ScalarizeLoadStoreKey,
> "ScalarizeLoadStore",
>   "scalarize-load-store", cl::Hidden, cl::init(false),
>   cl::desc("Allow the scalarizer pass to scalarize loads and
store"));
> ....
> bool ScalarizeLoadStore >
cl::OptionRegistry::getValue<bool>(&ScalarizeLoadStoreKey); // local
> 
> That way we avoid exposing ScalarizeLoadStore to library users since
> the getValue takes a key they cannot guess instead of well known
> string.
> 
> Cheers,
> Rafael
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

>> Pass the passinfo to the pass constructor maybe?
>>
>> I still don't understand what the problem with the global is. It
has
>> the same value for all users. As Chandler pointed out, having
>
> Globals are bad for many reasons: namespace pollution, concurrency issues,
lack of access control, etc. etc.. Some of them have been discussed in this
thread. Perhaps it’s not a big concern for most of the LLVM users. But we have
an unique environment where LLVM is shared by multiple clients, and where the
concern around exploits are especially strong. So while eliminating globals is
not strictly tied to the elimination of static initializers, it is still a
strong goal towards providing a LLVM dylib.
Fair enough. I would still make at separate stage since how to remove
them is the part that is still a bit contentious. Note that from a
security point of view a well known string probably makes things worse
than the global option storage since the address of the storage is at
least randomized.

It seems that both mine and Peter's proposal would avoid the global
storage and not introduce well known string for setting the options.

Cheers,
Rafael

I think Rafael’s suggestion of using the address of a global as the key to the
option store gives us the ability to accomplish all our goals without
introducing a string keyed option store.

I’ve updated my patches from the original proposal to reflect those changes.

Does this look reasonable?

Thanks,
-Chris
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cl_opt.diff
Type: application/octet-stream
Size: 9063 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140820/b1f47c9b/attachment.obj>
-------------- next part --------------
> On Aug 20, 2014, at 12:41 PM, Rafael Espíndola <rafael.espindola at
gmail.com> wrote:
> 
>>> Pass the passinfo to the pass constructor maybe?
>>> 
>>> I still don't understand what the problem with the global is.
It has
>>> the same value for all users. As Chandler pointed out, having
>> 
>> Globals are bad for many reasons: namespace pollution, concurrency
issues, lack of access control, etc. etc.. Some of them have been discussed in
this thread. Perhaps it’s not a big concern for most of the LLVM users. But we
have an unique environment where LLVM is shared by multiple clients, and where
the concern around exploits are especially strong. So while eliminating globals
is not strictly tied to the elimination of static initializers, it is still a
strong goal towards providing a LLVM dylib.
> 
> Fair enough. I would still make at separate stage since how to remove
> them is the part that is still a bit contentious. Note that from a
> security point of view a well known string probably makes things worse
> than the global option storage since the address of the storage is at
> least randomized.
> 
> It seems that both mine and Peter's proposal would avoid the global
> storage and not introduce well known string for setting the options.
> 
> Cheers,
> Rafael
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

I think Rafael’s suggestion of using the address of a global as the key to the
option store gives us the ability to accomplish all our goals without
introducing a string keyed option store.

I’ve updated my patches from the original proposal to reflect those changes.

Does this look reasonable?

Thanks,
-Chris
> On Aug 20, 2014, at 1:07 PM, Owen Anderson <resistor at mac.com>
wrote:
> 
> 
>> On Aug 20, 2014, at 12:16 PM, Rafael Avila de Espindola
<rafael.espindola at gmail.com <mailto:rafael.espindola at
gmail.com>> wrote:
>> 
>> There nothing magical about llc or opt. If you need to pass debug
options to your own tools, you can do that with command line options like llc or
opt, but the important factors remain:
>> 
>> * this is a debugging session, not regular use.
>> * the option storage can remain static.
>> * we don't need and API for setting the value of the option,
command line is sufficient.
> 
> #2 and #3 are not true.
> 
> •  As Chris has pointed out, it’s a perfectly normal circumstance in the
systems we deal with to have multiple instances of LLVM resident for different
clients.  Lots of applications bring in both WebKit and the OpenGL, including
the browser itself via WebGL!  If I’m debugging or testing one component of the
system, I definitely do not want debug options I have set to cause differences
in the other components.
> 
> • A lot of these users are not launched at the command line in a typical
fashion.  They’re either embedded into the address space of an existing
application (which may have a totally different command line interface, or
indeed none at all), or running in daemons whose lifetime is controlled by other
parts of the system.  Either way, setting options at the command line in not
always viable.
> 
> I agree that llc, opt and clang *shouldn’t* be special, but everything
you’re saying here is firmly rooted in a world where they are special, or at
least all use cases for debugging LLVM look like them.
> 
> —Owen
> 
> 
>> Sent from my iPhone
>> 
>> On Aug 20, 2014, at 13:42, Owen Anderson <resistor at mac.com
<mailto:resistor at mac.com>> wrote:
>> 
>>> 
>>>> On Aug 19, 2014, at 10:24 PM, Chandler Carruth <chandlerc at
google.com <mailto:chandlerc at google.com>> wrote:
>>>> 
>>>> 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 think this statement ignores important debugging use cases, and
over-simplifies the model of how LLVM-based frameworks are often developed.  As
an example, say I develop an LLVM backend for a custom co-processor, and the
compiler for it runs inside the software stack of the driver for my
co-processor.  Per what you and Rafael are saying, the driver should not be able
to programmatically set command line options to LLVM.  But this ignores the
reality that, when developing my backend, I may still need to be able to twiddle
debugging options when debugging a live driver stack.
>>> 
>>> Fundamentally, you argument is rooted in a world where LLVM
debugging is always done on the llc or clang binaries, where the LLVM developer
can easily modify the command line arguments themselves.  That’s not true of
many use cases where LLVM as a shared library is relevant.
>>> 
>>> —Owen
>>> _______________________________________________
>>> 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>
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140820/f1e3e323/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cl_opt.diff
Type: application/octet-stream
Size: 9063 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140820/f1e3e323/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140820/f1e3e323/attachment-0001.html>

On Wed, Aug 20, 2014 at 2:16 PM, Chris Bieneman <beanz at apple.com>
wrote:
> I think Rafael’s suggestion of using the address of a global as the key to
> the option store gives us the ability to accomplish all our goals without
> introducing a string keyed option store.
>
> I’ve updated my patches from the original proposal to reflect those
> changes.
>
> Does this look reasonable?
>
The design seems to require no less than 4 not-too-close-to-each-other code
changes to add an option (and LLVMOptionKey declaration, an LLVMOptionKey
initialization, a call to GetValue, and a call to CreateOption). I think
that the convenience for developers is an important aspect and this
solution regresses significantly on that front.

Especially, I think that we need to evaluate the viability of an explicit
CreateOption call. Do all uses of cl::opt in the library code have a
relatively convenient place to put that call? If not, then we won't be able
to migrate them, and we will be stuck with a partial solution. If we are
willing to accept a partial solution, then the design discussion needs to
change; at the very least we need to consider the scope of the partial
solution.

-- Sean Silva

>
> Thanks,
> -Chris
>
>
> On Aug 20, 2014, at 1:07 PM, Owen Anderson <resistor at mac.com>
wrote:
>
>
> On Aug 20, 2014, at 12:16 PM, Rafael Avila de Espindola <
> rafael.espindola at gmail.com> wrote:
>
> There nothing magical about llc or opt. If you need to pass debug options
> to your own tools, you can do that with command line options like llc or
> opt, but the important factors remain:
>
> * this is a debugging session, not regular use.
> * the option storage can remain static.
> * we don't need and API for setting the value of the option, command
line
> is sufficient.
>
>
> #2 and #3 are not true.
>
> •  As Chris has pointed out, it’s a perfectly normal circumstance in the
> systems we deal with to have multiple instances of LLVM resident for
> different clients.  Lots of applications bring in both WebKit and the
> OpenGL, including the browser itself via WebGL!  If I’m debugging or
> testing one component of the system, I definitely do not want debug options
> I have set to cause differences in the other components.
>
> • A lot of these users are not launched at the command line in a typical
> fashion.  They’re either embedded into the address space of an existing
> application (which may have a totally different command line interface, or
> indeed none at all), or running in daemons whose lifetime is controlled by
> other parts of the system.  Either way, setting options at the command line
> in not always viable.
>
> I agree that llc, opt and clang *shouldn’t* be special, but everything
> you’re saying here is firmly rooted in a world where they are special, or
> at least all use cases for debugging LLVM look like them.
>
> —Owen
>
>
> Sent from my iPhone
>
> On Aug 20, 2014, at 13:42, Owen Anderson <resistor at mac.com> wrote:
>
>
> On Aug 19, 2014, at 10:24 PM, Chandler Carruth <chandlerc at
google.com>
> wrote:
>
> 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 think this statement ignores important debugging use cases, and
> over-simplifies the model of how LLVM-based frameworks are often developed.
>  As an example, say I develop an LLVM backend for a custom co-processor,
> and the compiler for it runs inside the software stack of the driver for my
> co-processor.  Per what you and Rafael are saying, the driver should not be
> able to programmatically set command line options to LLVM.  But this
> ignores the reality that, when developing my backend, I may still need to
> be able to twiddle debugging options when debugging a live driver stack.
>
> Fundamentally, you argument is rooted in a world where LLVM debugging is
> always done on the llc or clang binaries, where the LLVM developer can
> easily modify the command line arguments themselves.  That’s not true of
> many use cases where LLVM as a shared library is relevant.
>
> —Owen
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140820/75fe7e4c/attachment.html>

> On Aug 20, 2014, at 3:12 PM, Sean Silva <chisophugis at gmail.com>
wrote:
> 
> 
> 
> 
> On Wed, Aug 20, 2014 at 2:16 PM, Chris Bieneman <beanz at apple.com
<mailto:beanz at apple.com>> wrote:
> I think Rafael’s suggestion of using the address of a global as the key to
the option store gives us the ability to accomplish all our goals without
introducing a string keyed option store.
> 
> I’ve updated my patches from the original proposal to reflect those
changes.
> 
> Does this look reasonable?
> 
> The design seems to require no less than 4 not-too-close-to-each-other code
changes to add an option (and LLVMOptionKey declaration, an LLVMOptionKey
initialization, a call to GetValue, and a call to CreateOption). I think that
the convenience for developers is an important aspect and this solution
regresses significantly on that front.
> 
> Especially, I think that we need to evaluate the viability of an explicit
CreateOption call. Do all uses of cl::opt in the library code have a relatively
convenient place to put that call? If not, then we won't be able to migrate
them, and we will be stuck with a partial solution. If we are willing to accept
a partial solution, then the design discussion needs to change; at the very
least we need to consider the scope of the partial solution.
Most cl::opts are in the passes, so they do have a convenient place to put the
CreateOption call. There are others in places that are less convenient - like
ISel lowering. However there are always initialization calls that CreateOption
calls can be put into (in targets, libraries, etc).

Fundamentally you and I are looking at this problem differently. My proposal has
the following requirements:
1) Preserve existing command line option function exactly as it is today
2) Eliminate all static initializers associated with command line options and
their storage
3) Design a solution that can be phased in with existing code so we don’t need
to change all cl::opts in the same commit

These requirements unfortunately made your requirements unfeasible.

One important point I would make, is that while my changes may make it slightly
less convenient to create new command line options, it does not impact the
ability of a developer to manipulate the values of the options. The flags behave
as they do today, and there is still one line of code somewhere that sets the
value the compiler actually uses. In my example this is in the constructor for
the pass.

-Chris
> 
> -- Sean Silva
>  
> 
> Thanks,
> -Chris
> 
> 
>> On Aug 20, 2014, at 1:07 PM, Owen Anderson <resistor at mac.com
<mailto:resistor at mac.com>> wrote:
>> 
>> 
>>> On Aug 20, 2014, at 12:16 PM, Rafael Avila de Espindola
<rafael.espindola at gmail.com <mailto:rafael.espindola at
gmail.com>> wrote:
>>> 
>>> There nothing magical about llc or opt. If you need to pass debug
options to your own tools, you can do that with command line options like llc or
opt, but the important factors remain:
>>> 
>>> * this is a debugging session, not regular use.
>>> * the option storage can remain static.
>>> * we don't need and API for setting the value of the option,
command line is sufficient.
>> 
>> #2 and #3 are not true.
>> 
>> •  As Chris has pointed out, it’s a perfectly normal circumstance in
the systems we deal with to have multiple instances of LLVM resident for
different clients.  Lots of applications bring in both WebKit and the OpenGL,
including the browser itself via WebGL!  If I’m debugging or testing one
component of the system, I definitely do not want debug options I have set to
cause differences in the other components.
>> 
>> • A lot of these users are not launched at the command line in a
typical fashion.  They’re either embedded into the address space of an existing
application (which may have a totally different command line interface, or
indeed none at all), or running in daemons whose lifetime is controlled by other
parts of the system.  Either way, setting options at the command line in not
always viable.
>> 
>> I agree that llc, opt and clang *shouldn’t* be special, but everything
you’re saying here is firmly rooted in a world where they are special, or at
least all use cases for debugging LLVM look like them.
>> 
>> —Owen
>> 
>> 
>>> Sent from my iPhone
>>> 
>>> On Aug 20, 2014, at 13:42, Owen Anderson <resistor at mac.com
<mailto:resistor at mac.com>> wrote:
>>> 
>>>> 
>>>>> On Aug 19, 2014, at 10:24 PM, Chandler Carruth
<chandlerc at google.com <mailto:chandlerc at google.com>> wrote:
>>>>> 
>>>>> 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 think this statement ignores important debugging use cases,
and over-simplifies the model of how LLVM-based frameworks are often developed. 
As an example, say I develop an LLVM backend for a custom co-processor, and the
compiler for it runs inside the software stack of the driver for my
co-processor.  Per what you and Rafael are saying, the driver should not be able
to programmatically set command line options to LLVM.  But this ignores the
reality that, when developing my backend, I may still need to be able to twiddle
debugging options when debugging a live driver stack.
>>>> 
>>>> Fundamentally, you argument is rooted in a world where LLVM
debugging is always done on the llc or clang binaries, where the LLVM developer
can easily modify the command line arguments themselves.  That’s not true of
many use cases where LLVM as a shared library is relevant.
>>>> 
>>>> —Owen
>>>> _______________________________________________
>>>> 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>
>> 
>> _______________________________________________
>> 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>
> 
> 
> _______________________________________________
> 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/20140820/328147db/attachment.html>

On Wed, Aug 20, 2014 at 3:53 PM, Chris Bieneman <beanz at apple.com>
wrote:
>
> On Aug 20, 2014, at 3:12 PM, Sean Silva <chisophugis at gmail.com>
wrote:
>
>
>
>
> On Wed, Aug 20, 2014 at 2:16 PM, Chris Bieneman <beanz at apple.com>
wrote:
>
>> I think Rafael’s suggestion of using the address of a global as the key
>> to the option store gives us the ability to accomplish all our goals
>> without introducing a string keyed option store.
>>
>> I’ve updated my patches from the original proposal to reflect those
>> changes.
>>
>> Does this look reasonable?
>>
>
> The design seems to require no less than 4 not-too-close-to-each-other
> code changes to add an option (and LLVMOptionKey declaration, an
> LLVMOptionKey initialization, a call to GetValue, and a call to
> CreateOption). I think that the convenience for developers is an important
> aspect and this solution regresses significantly on that front.
>
> Especially, I think that we need to evaluate the viability of an explicit
> CreateOption call. Do all uses of cl::opt in the library code have a
> relatively convenient place to put that call? If not, then we won't be
able
> to migrate them, and we will be stuck with a partial solution. If we are
> willing to accept a partial solution, then the design discussion needs to
> change; at the very least we need to consider the scope of the partial
> solution.
>
>
> Most cl::opts are in the passes, so they do have a convenient place to put
> the CreateOption call. There are others in places that are less convenient
> - like ISel lowering. However there are always initialization calls that
> CreateOption calls can be put into (in targets, libraries, etc).
>
> Fundamentally you and I are looking at this problem differently. My
> proposal has the following requirements:
> 1) Preserve existing command line option function *exactly* as it is today
> 2) Eliminate all static initializers associated with command line options
> and their storage
> 3) Design a solution that can be phased in with existing code so we don’t
> need to change all cl::opts in the same commit
>
I think your approach makes sense in view of those requirements.

-- Sean Silva

>
> These requirements unfortunately made your requirements unfeasible.
>
> One important point I would make, is that while my changes may make it
> slightly less convenient to create new command line options, it does not
> impact the ability of a developer to manipulate the values of the options.
> The flags behave as they do today, and there is still one line of code
> somewhere that sets the value the compiler actually uses. In my example
> this is in the constructor for the pass.
>
> -Chris
>
>
> -- Sean Silva
>
>
>>
>> Thanks,
>> -Chris
>>
>>
>> On Aug 20, 2014, at 1:07 PM, Owen Anderson <resistor at mac.com>
wrote:
>>
>>
>> On Aug 20, 2014, at 12:16 PM, Rafael Avila de Espindola <
>> rafael.espindola at gmail.com> wrote:
>>
>> There nothing magical about llc or opt. If you need to pass debug
options
>> to your own tools, you can do that with command line options like llc
or
>> opt, but the important factors remain:
>>
>> * this is a debugging session, not regular use.
>> * the option storage can remain static.
>> * we don't need and API for setting the value of the option,
command line
>> is sufficient.
>>
>>
>> #2 and #3 are not true.
>>
>> •  As Chris has pointed out, it’s a perfectly normal circumstance in
the
>> systems we deal with to have multiple instances of LLVM resident for
>> different clients.  Lots of applications bring in both WebKit and the
>> OpenGL, including the browser itself via WebGL!  If I’m debugging or
>> testing one component of the system, I definitely do not want debug
options
>> I have set to cause differences in the other components.
>>
>> • A lot of these users are not launched at the command line in a
typical
>> fashion.  They’re either embedded into the address space of an existing
>> application (which may have a totally different command line interface,
or
>> indeed none at all), or running in daemons whose lifetime is controlled
by
>> other parts of the system.  Either way, setting options at the command
line
>> in not always viable.
>>
>> I agree that llc, opt and clang *shouldn’t* be special, but everything
>> you’re saying here is firmly rooted in a world where they are special,
or
>> at least all use cases for debugging LLVM look like them.
>>
>> —Owen
>>
>>
>> Sent from my iPhone
>>
>> On Aug 20, 2014, at 13:42, Owen Anderson <resistor at mac.com>
wrote:
>>
>>
>> On Aug 19, 2014, at 10:24 PM, Chandler Carruth <chandlerc at
google.com>
>> wrote:
>>
>> 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 think this statement ignores important debugging use cases, and
>> over-simplifies the model of how LLVM-based frameworks are often
developed.
>>  As an example, say I develop an LLVM backend for a custom
co-processor,
>> and the compiler for it runs inside the software stack of the driver
for my
>> co-processor.  Per what you and Rafael are saying, the driver should
not be
>> able to programmatically set command line options to LLVM.  But this
>> ignores the reality that, when developing my backend, I may still need
to
>> be able to twiddle debugging options when debugging a live driver
stack.
>>
>> Fundamentally, you argument is rooted in a world where LLVM debugging
is
>> always done on the llc or clang binaries, where the LLVM developer can
>> easily modify the command line arguments themselves.  That’s not true
of
>> many use cases where LLVM as a shared library is relevant.
>>
>> —Owen
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140820/b21e2926/attachment.html>

I don't have any objections to this approach. Just some comments:

+  LLVMOptionKey* Key; // The name of the option used by the OptionRegistry

The comment looks out of date.

+  std::map<LLVMOptionKey*,Option*> Options;

Why std::map? DenseMap maybe?

+  void AddOption(LLVMOptionKey* key, Option *O);
+  void RemoveOption(Option *O);

addOption, removeOption. Same for other methods.

+LLVMOptionKey Scalarizer::ScalarizeLoadStoreKey = 0;

the value is not used, so I would probably omit the "= 0;".

I'll try to provide some feedback on this patch assuming that we want to
try to completely move away from global variables in a single step
(something that I do generally support, although I don't understand why a
more incremental approach is unacceptable -- it seems like a bit more work
over all but a somewhat smoother transition for the community).

I feel like the APIs you're ending up with are bending around backwards to
achieve things that it isn't even clear are valuable:

1) They seem to be trying to working very hard to behave similarly to the
existing cl::opt interfaces. If we're so radically changing the core design
of cl::opt, different interfaces might be in order.

2) Perhaps this was explained up-thread (I'm going to try to read most of
the older posts, but I'm still catching up on this thread) but if we're
going this direction I would really expect the options to be
per-LLVMContext... I feel like these are just "lazy globals with private
storage" which don't seem all that conceptually better than globals.
They've just engineered around the technical problems caused.

3) I really dislike the whole INITIALIZE_PASS_{BEGIN,END} pattern. I'm not
sure if there is any realistic alternative, but I'm expecting to
essentially have to rework every single one of these for the new pass
manager. =[


I'm going to try to read Sean's proposal, but I'm suspect it is
essentially
a strongly typed option registry inside the LLVMContext? With some way to
collect the options first and then to parse flags? That kind of approach
seems more likely to really solve this issue once and for all and be an
excellent, and scalable long-term solution... but as I said, I'm still
catching up on the thread. I'll try to post more coherent thoughts when
I've had time to read the other posts fully and skim through the other
proposals.



On Wed, Aug 20, 2014 at 12:45 PM, Chris Bieneman <cbieneman at apple.com>
wrote:
> I think Rafael’s suggestion of using the address of a global as the key to
> the option store gives us the ability to accomplish all our goals without
> introducing a string keyed option store.
>
> I’ve updated my patches from the original proposal to reflect those
> changes.
>
> Does this look reasonable?
>
> Thanks,
> -Chris
>
>
> > On Aug 20, 2014, at 12:41 PM, Rafael Espíndola <
> rafael.espindola at gmail.com> wrote:
> >
> >>> Pass the passinfo to the pass constructor maybe?
> >>>
> >>> I still don't understand what the problem with the global
is. It has
> >>> the same value for all users. As Chandler pointed out, having
> >>
> >> Globals are bad for many reasons: namespace pollution, concurrency
> issues, lack of access control, etc. etc.. Some of them have been discussed
> in this thread. Perhaps it’s not a big concern for most of the LLVM users.
> But we have an unique environment where LLVM is shared by multiple clients,
> and where the concern around exploits are especially strong. So while
> eliminating globals is not strictly tied to the elimination of static
> initializers, it is still a strong goal towards providing a LLVM dylib.
> >
> > Fair enough. I would still make at separate stage since how to remove
> > them is the part that is still a bit contentious. Note that from a
> > security point of view a well known string probably makes things worse
> > than the global option storage since the address of the storage is at
> > least randomized.
> >
> > It seems that both mine and Peter's proposal would avoid the
global
> > storage and not introduce well known string for setting the options.
> >
> > Cheers,
> > Rafael
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140820/4b968ecb/attachment.html>

Thanks Chandler. I would very much appreciate having your feedback on this.
> On Aug 20, 2014, at 11:18 PM, Chandler Carruth <chandlerc at
google.com> wrote:
> 
> I'll try to provide some feedback on this patch assuming that we want
to try to completely move away from global variables in a single step (something
that I do generally support, although I don't understand why a more
incremental approach is unacceptable -- it seems like a bit more work over all
but a somewhat smoother transition for the community).
> 
> I feel like the APIs you're ending up with are bending around backwards
to achieve things that it isn't even clear are valuable:
> 
> 1) They seem to be trying to working very hard to behave similarly to the
existing cl::opt interfaces. If we're so radically changing the core design
of cl::opt, different interfaces might be in order.
I wouldn’t say the APIs are working hard to behave similarly to the existing
cl::opt interfaces as much as I designed this patch to provide incremental
improvements to cl::opt, and changing the API was not on the table for the first
change.

The primary focus of this change is to provide a new way of defining existing
cl::opt objects such that they are not globally scoped. This change should not
preclude us from coming up with a new better API for options, but that should be
a separate discussion from using the existing API while avoiding some of the
less desirable side-effects.
> 
> 2) Perhaps this was explained up-thread (I'm going to try to read most
of the older posts, but I'm still catching up on this thread) but if
we're going this direction I would really expect the options to be
per-LLVMContext... I feel like these are just "lazy globals with private
storage" which don't seem all that conceptually better than globals.
They've just engineered around the technical problems caused.
Earlier in the thread we’ve danced around this. My original proposal was to
eventually create an option store object that would be passed into the context,
but would be independent of it. The reason for not putting it in the context is
because largest consumers of command line options are passes, and we already
have use cases where we re-use pass managers with different contexts. Due to
some early feedback on the thread I thought it would be best to shelve that
conversation for a later date, but I’d love to know what feedback you have in
that area.
> 
> 3) I really dislike the whole INITIALIZE_PASS_{BEGIN,END} pattern. I'm
not sure if there is any realistic alternative, but I'm expecting to
essentially have to rework every single one of these for the new pass manager.
=[
I agree the INITIALIZE_PASS macros are nasty, but fundamentally I think you
either need to be okay with global initializers (which we’re not), or you need
to have some form of an explicit initialization.
> 
> 
> I'm going to try to read Sean's proposal, but I'm suspect it is
essentially a strongly typed option registry inside the LLVMContext? With some
way to collect the options first and then to parse flags? That kind of approach
seems more likely to really solve this issue once and for all and be an
excellent, and scalable long-term solution... but as I said, I'm still
catching up on the thread. I'll try to post more coherent thoughts when
I've had time to read the other posts fully and skim through the other
proposals.
I don’t disagree that there are merits to Sean’s proposal, but there are also
issues. I’d be more than happy to debate this in more detail, but I would
actually ask that we set it aside for now. I’d really prefer if we didn’t gate
removing static initializers and global option storage, which I think we can
both agree are good things, on a larger change to a core design pattern within
LLVM.

Thanks,
-Chris
> 
> 
> 
> On Wed, Aug 20, 2014 at 12:45 PM, Chris Bieneman <cbieneman at apple.com
<mailto:cbieneman at apple.com>> wrote:
> I think Rafael’s suggestion of using the address of a global as the key to
the option store gives us the ability to accomplish all our goals without
introducing a string keyed option store.
> 
> I’ve updated my patches from the original proposal to reflect those
changes.
> 
> Does this look reasonable?
> 
> Thanks,
> -Chris
> 
> 
> > On Aug 20, 2014, at 12:41 PM, Rafael Espíndola <rafael.espindola at
gmail.com <mailto:rafael.espindola at gmail.com>> wrote:
> >
> >>> Pass the passinfo to the pass constructor maybe?
> >>>
> >>> I still don't understand what the problem with the global
is. It has
> >>> the same value for all users. As Chandler pointed out, having
> >>
> >> Globals are bad for many reasons: namespace pollution, concurrency
issues, lack of access control, etc. etc.. Some of them have been discussed in
this thread. Perhaps it’s not a big concern for most of the LLVM users. But we
have an unique environment where LLVM is shared by multiple clients, and where
the concern around exploits are especially strong. So while eliminating globals
is not strictly tied to the elimination of static initializers, it is still a
strong goal towards providing a LLVM dylib.
> >
> > Fair enough. I would still make at separate stage since how to remove
> > them is the part that is still a bit contentious. Note that from a
> > security point of view a well known string probably makes things worse
> > than the global option storage since the address of the storage is at
> > least randomized.
> >
> > It seems that both mine and Peter's proposal would avoid the
global
> > storage and not introduce well known string for setting the options.
> >
> > 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>
> 
> 
> _______________________________________________
> 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/20140821/2119c3bb/attachment.html>

On Thu, Aug 21, 2014 at 10:16 AM, Chris Bieneman <beanz at apple.com>
wrote:
> I’d really prefer if we didn’t gate removing static initializers and
> global option storage, which I think we can both agree are good things, on
> a larger change to a core design pattern within LLVM.

FWIW, while I think it may be possible to remove static initializers
without changing core design patterns within LLVM, I don't think that's
the
case for removing global storage. Quite fundamentally, once you're not
using global storage I think you want a *different* interface to these
things. =/
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140821/bcd5dc74/attachment.html>

The problem that led me to tackling both together was that there are certain
command line options that we use today where the storage itself will incur
static initializers if it is global. Examples of these are command line options
where the stores are lists, strings, and classes.

The fact that our options support these types makes the problem of eliminating
static initializers bound to the problem of eliminating global storage. This
problem also gets more complicated when you take into account that the logical
place to move the storage for many of these options (the passes themselves)
don’t actually exist at the time that you are parsing the options.

If you have suggestions on how to tackle these problems I’d be grateful for the
feedback.

-Chris
> On Aug 21, 2014, at 10:33 AM, Chandler Carruth <chandlerc at
google.com> wrote:
> 
> 
> On Thu, Aug 21, 2014 at 10:16 AM, Chris Bieneman <beanz at apple.com
<mailto:beanz at apple.com>> wrote:
> I’d really prefer if we didn’t gate removing static initializers and global
option storage, which I think we can both agree are good things, on a larger
change to a core design pattern within LLVM.
> 
> FWIW, while I think it may be possible to remove static initializers
without changing core design patterns within LLVM, I don't think that's
the case for removing global storage. Quite fundamentally, once you're not
using global storage I think you want a *different* interface to these things.
=/
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140821/b3a9d814/attachment.html>

A bunch of IRC discussions etc have taken place, and I wanted to surface
some of the points from them here. Also, I wanted to give a more fully
thought-through set of my own perspectives.


First, I'd like to re-state the goals as I think those have gotten muddied.

1) Remove most of LLVM's static initializers (that is, dynamic
initialization code for static storage duration objects) by moving away
from global objects with non-trivial constructors of 'llvm::cl::opt'
types
in the LLVM *libraries* (they remain fine in the tools of course).

2) Support separate LLVM users in a single address space having different
values for the debugging knobs currently managed through the
'llvm::cl::opt' command line flags mechanism.

3) (related to #2) Support some library API for setting the debugging knobs
currently managed through the 'llvm::cl::opt' command line flags
mechanism.


Now, I want to change my position on one point because after talking to
Chris, Jim, and others I understand the problem significantly better. I am
now convinced that it is impossible to fix even #1 without making a change
for every single option in LLVM. Why? Because we have to *register* all of
the flags, and this registration inherently cannot be done from a global's
constructor without introducing the undesirable static initializers[1].

I also think that in order to address #2 we would need to make some change
to essentially every option in LLVM as we would have to connect it to
non-global storage.

Given that, I think it does make sense to try to address #1 and #2 at the
same time -- trying to limit the change to just one or the other would very
likely result in a significantly different API for creating, registering,
and using these options, and I would rather not make all of the LLVM
developers re-learn how to do this 2 times if we can instead do it 1 time.


However, doing #2 only really makes sense if we also do #3 to let people
*use* this new freedom. =] Owen convinced me that there really is a good
reason to support #3 as part of the LLVM libraries, however I wanted to
raise my primary concern about this on the mailing list because it hasn't
really been discussed.

These "debugging knobs" or library level options which *aren't*
exposed in
the library's APIs are very risky to expose in a side-channel API: they are
often completely unsupported or unsupportable options that were never
intended for "production" usage. Once we expose these options via some
API
to library users, we run a very serious risk of getting locked into
supporting behaviors or debugging misbehavior that were never expected or
planned for...

However, the debugging use case is real and important. So I want to propose
that we guard many of the paths to setting these options with NDEBUG so
that in non-asserts builds, library users can't override these options.
This would mean the C++ API for setting this in LLVMContext (and the C API
if we ever expose it there) would only support setting these in asserts
builds. We could even sink the checking into the registry. The name should
probably have "debug" in it somewhere. It would then have some
InternalUnsupportedScaryBad API which is not actually in the header files
but is used in LLVM's tools to maintain their current behavior without
documenting a public API. It's not bulletproof, but it seemed that it would
be sufficient for the debugging use cases and is a strong statement of
"unsupported" IMO.


The only other observation I would like to make is that if we're going to
do this, we really should take the time to try to sort out a really *good*
API. Hopefully we'll be slightly less wrong this time, and maybe the next
transition can be more incremental.


Ok, on to the more low-level parts of this email, here some specific
thoughts for how to design the API for further discussion:

- I really like the idea of using a 'void*' derived from some static tag
as
a way to identify uniquely each option. I also think we can use some
minimal "reflection" techniques to significantly simplify this.

- The need for a global registry is... distasteful, but I agree inevitable.

- I'd like to avoid a separate "option store". My current
suggestion is to
use the LLVMContext, and to add a context to the layers which don't have an
LLVMContext and where these kinds of options make sense.

- I would actually use a simple 'void*' -> string mapping in the
"store" of
the context. (Maybe we can actually use StringMap?) I would still provide
the type to the registry and use that to validate options when parsing.
When getting the value out of the "store" in the context, we can parse
it
into the data-type and assert fail on parse errors. I don't feel strongly
about this, but without "std::any" it seems simpler than rolling our
own
with a void* or a union.

- I would provide a simple API on the LLVMContext to get an "option"
string
for a given key. This API could be exposed by any context, and we can write
the option value extraction library to accept a generic T which supports
such a query method. That way there is *very* loose coupling between these
and it is easy to support any context object at any layer.

- The registry should essentially provide an interface to parse an argv
string list into such a map from 'void*' -> string. The LLVMContext
(or any
other context) can then be populated by the result of this parse. Each
library can parse whatever argv it wants.

- I don't think we should keep piling on the interface design of the
existing cl::opt stuff. I would make a clean break to a new header /
library, and just build some code to map these into the cl::opt parsing
code from the registry (we already do stuff like this for pass names). This
will also help the two systems co-exist but stay separate during the
(inevitably long) transition period.

- As a specific example of what I would change from the cl::opt stuff: I
think we should make the required arguments just be required and
positional, and the optional arguments use a fluent-style interface (yea, I
know, but I think it works well here) rather than the weird variadic-ish
thing...


Once there is a patch that seems like a reasonable start at this, I'd
actually like to get the new library added and just one pass (the one
you're using seems great) converted. I'd then like to try a few
experiments
to see if we can improve some of the nitty-gritty details of the API before
we roll it out widely.

Seem reasonable? I'm actually happy to help with some of the API
experiments and the rollout here.
-Chandler


[1]: I'd actually like to understand why on some platforms the explicit
global initialization routines are so much better than global
constructors... But that's probably a topic for on off-list or IRC
conversation. I don't want to derail this thread with that.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140826/189c99c2/attachment.html>

> On Aug 26, 2014, at 11:52 AM, Rafael Espíndola <rafael.espindola at
gmail.com> wrote:
> 
>> These "debugging knobs" or library level options which
*aren't* exposed in
>> the library's APIs are very risky to expose in a side-channel API:
they are
>> often completely unsupported or unsupportable options that were never
>> intended for "production" usage. Once we expose these options
via some API
>> to library users, we run a very serious risk of getting locked into
>> supporting behaviors or debugging misbehavior that were never expected
or
>> planned for...
> 
> Agreed. This is my only real concern in here.
> 
>> However, the debugging use case is real and important. So I want to
propose
>> that we guard many of the paths to setting these options with NDEBUG so
that
>> in non-asserts builds, library users can't override these options.
This
>> would mean the C++ API for setting this in LLVMContext (and the C API
if we
>> ever expose it there) would only support setting these in asserts
builds. We
>> could even sink the checking into the registry. The name should
probably
>> have "debug" in it somewhere. It would then have some
>> InternalUnsupportedScaryBad API which is not actually in the header
files
>> but is used in LLVM's tools to maintain their current behavior
without
>> documenting a public API. It's not bulletproof, but it seemed that
it would
>> be sufficient for the debugging use cases and is a strong statement of
>> "unsupported" IMO.
> 
> Just to be clear, the idea is that in a release build the only way of
> setting options is via the command line? That they can still be passed
> to opt or llc, but they would not be considered part of the API.
> 
>> - I really like the idea of using a 'void*' derived from some
static tag as
>> a way to identify uniquely each option. I also think we can use some
minimal
>> "reflection" techniques to significantly simplify this.
> 
> I like it too. The reason I do is that it is one more layer in a
> defense in depth against making our internal options part of the API.
> The key is not just a well known value (like a string).
> 
>> - I'd like to avoid a separate "option store". My current
suggestion is to
>> use the LLVMContext, and to add a context to the layers which don't
have an
>> LLVMContext and where these kinds of options make sense.
> 
> I agree. There are few places in LLVM without a LLVMContext. For
> example, there is one cl::opt in lib/MC and Joerg already has a patch
> to turn it into a proper API.
I disagree with this point.  The majority of the fields of the context are IR. 
An MC only tool should not be required to link the LLVM IR just because they
want to use a command line option.

Chris’ suggestion is to make an OptionStore class and pass it throughout the
APIs where we want to handle options.  At first the store should be a global
singleton.  Users of options don’t need to know about the LLVMContext or any
other unrelated data structure.  If the options are stored as a field of the
context then that can easily be done too.

If we later decide that the OptionStore should live in the LLVMContext or any
other location then thats fine, but moving it to that point would be a minimal
change as no APIs have to be rewritten.  Also, if we decide that we want the
LLVMContext to have an option store, but MC level tools to be allowed to define
their own, then that would also be possible.

Thanks,
Pete
> 
> Cheers,
> Rafael
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

On Tue, Aug 26, 2014 at 5:09 PM, Pete Cooper <peter_cooper at apple.com>
wrote:
>
> > On Aug 26, 2014, at 11:52 AM, Rafael Espíndola <
> rafael.espindola at gmail.com> wrote:
> >
> >> These "debugging knobs" or library level options which
*aren't* exposed
> in
> >> the library's APIs are very risky to expose in a side-channel
API: they
> are
> >> often completely unsupported or unsupportable options that were
never
> >> intended for "production" usage. Once we expose these
options via some
> API
> >> to library users, we run a very serious risk of getting locked
into
> >> supporting behaviors or debugging misbehavior that were never
expected
> or
> >> planned for...
> >
> > Agreed. This is my only real concern in here.
> >
> >> However, the debugging use case is real and important. So I want
to
> propose
> >> that we guard many of the paths to setting these options with
NDEBUG so
> that
> >> in non-asserts builds, library users can't override these
options. This
> >> would mean the C++ API for setting this in LLVMContext (and the C
API
> if we
> >> ever expose it there) would only support setting these in asserts
> builds. We
> >> could even sink the checking into the registry. The name should
probably
> >> have "debug" in it somewhere. It would then have some
> >> InternalUnsupportedScaryBad API which is not actually in the
header
> files
> >> but is used in LLVM's tools to maintain their current behavior
without
> >> documenting a public API. It's not bulletproof, but it seemed
that it
> would
> >> be sufficient for the debugging use cases and is a strong
statement of
> >> "unsupported" IMO.
> >
> > Just to be clear, the idea is that in a release build the only way of
> > setting options is via the command line? That they can still be passed
> > to opt or llc, but they would not be considered part of the API.
> >
> >> - I really like the idea of using a 'void*' derived from
some static
> tag as
> >> a way to identify uniquely each option. I also think we can use
some
> minimal
> >> "reflection" techniques to significantly simplify this.
> >
> > I like it too. The reason I do is that it is one more layer in a
> > defense in depth against making our internal options part of the API.
> > The key is not just a well known value (like a string).
> >
> >> - I'd like to avoid a separate "option store". My
current suggestion is
> to
> >> use the LLVMContext, and to add a context to the layers which
don't
> have an
> >> LLVMContext and where these kinds of options make sense.
> >
> > I agree. There are few places in LLVM without a LLVMContext. For
> > example, there is one cl::opt in lib/MC and Joerg already has a patch
> > to turn it into a proper API.
> I disagree with this point.  The majority of the fields of the context are
> IR.  An MC only tool should not be required to link the LLVM IR just
> because they want to use a command line option.
>
> Chris’ suggestion is to make an OptionStore class and pass it throughout
> the APIs where we want to handle options.  At first the store should be a
> global singleton.  Users of options don’t need to know about the
> LLVMContext or any other unrelated data structure.  If the options are
> stored as a field of the context then that can easily be done too.
>
> If we later decide that the OptionStore should live in the LLVMContext or
> any other location then thats fine, but moving it to that point would be a
> minimal change as no APIs have to be rewritten.  Also, if we decide that we
> want the LLVMContext to have an option store, but MC level tools to be
> allowed to define their own, then that would also be possible.
>
The way I understand it, Chandler's proposal is to let any object act as an
object store if it satisfies the interface requirements (tested through
SFINAE or whatever). I'm guessing that whatever method the LLVMContext uses
to store the options will probably be reusable.

Just using a concrete ObjectStore class might be simpler than a more
loosely-coupled but more complex metaprogramming based approach (which as I
understand it is his suggestion).

-- Sean Silva

>
> Thanks,
> Pete
> >
> > Cheers,
> > Rafael
> > _______________________________________________
> > LLVM Developers mailing list
> > LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140826/7a8cd9dd/attachment.html>

On Tue, Aug 26, 2014 at 5:09 PM, Pete Cooper <peter_cooper at apple.com>
wrote:
> >> - I'd like to avoid a separate "option store". My
current suggestion is
> to
> >> use the LLVMContext, and to add a context to the layers which
don't
> have an
> >> LLVMContext and where these kinds of options make sense.
> >
> > I agree. There are few places in LLVM without a LLVMContext. For
> > example, there is one cl::opt in lib/MC and Joerg already has a patch
> > to turn it into a proper API.
> I disagree with this point.  The majority of the fields of the context are
> IR.  An MC only tool should not be required to link the LLVM IR just
> because they want to use a command line option.

Sorry, this is my fault, I phrased something poorly.

When I said "add a context to the layers ..." I didn't mean to add
an
LLVMContext to them. I agree, that's crazy. =] I meant add some context
object *for* that layer. Maybe an MCContext. This would *only* traffic in
the shared context and state needed by that layer, not any other layer. If
the layer needs these debug options, they can expose a generic interface
from that context and the option management code will happily use it.

The reason I mention this is that I don't want us to develop N different
objects which are essentially "side cars" to carry additional state
around
a layer of abstraction. We've been using LLVMContext to do that
successfully at the IR layer. At other layers where we need to do the same,
I'd like to add a single context *type* to represent that layer, and use
objects of that type to pass around both options and any other common
structures needed at that layer.

-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140826/5bc2304d/attachment.html>

> On Aug 26, 2014, at 5:31 PM, Chandler Carruth <chandlerc at
google.com> wrote:
> 
> 
> On Tue, Aug 26, 2014 at 5:09 PM, Pete Cooper <peter_cooper at apple.com
<mailto:peter_cooper at apple.com>> wrote:
> >> - I'd like to avoid a separate "option store". My
current suggestion is to
> >> use the LLVMContext, and to add a context to the layers which
don't have an
> >> LLVMContext and where these kinds of options make sense.
> >
> > I agree. There are few places in LLVM without a LLVMContext. For
> > example, there is one cl::opt in lib/MC and Joerg already has a patch
> > to turn it into a proper API.
> I disagree with this point.  The majority of the fields of the context are
IR.  An MC only tool should not be required to link the LLVM IR just because
they want to use a command line option.
> 
> Sorry, this is my fault, I phrased something poorly.
> 
> When I said "add a context to the layers ..." I didn't mean
to add an LLVMContext to them. I agree, that's crazy. =] I meant add some
context object *for* that layer. Maybe an MCContext. This would *only* traffic
in the shared context and state needed by that layer, not any other layer. If
the layer needs these debug options, they can expose a generic interface from
that context and the option management code will happily use it.
> 
> The reason I mention this is that I don't want us to develop N
different objects which are essentially "side cars" to carry
additional state around a layer of abstraction. We've been using LLVMContext
to do that successfully at the IR layer. At other layers where we need to do the
same, I'd like to add a single context *type* to represent that layer, and
use objects of that type to pass around both options and any other common
structures needed at that layer.
Sounds good.  Thanks for clarifying that.

Thanks,
Pete
> 
> -Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140826/c8c47488/attachment.html>

Chandler brought up a lot of really great points. One of his big points was that
if we’re going to touch every pass, we may as well fix the cl::opt API too.

Toward that end I’ve put together some patches with a new fluent-style API as a
starting point of a conversation.

Disclaimer: To make these patches work with the existing cl::opts there is some
nasty hackery going on in CommandLine.h — For the sake of this conversation I’d
like to focus our attention on the API for creating and accessing options in
Scalarizer.cpp

My intent with these changes is to provide a groundwork for meeting all of
Chandler’s main points. These patches have the following high-level changes from
my previous patches:

1) I’ve switched to some crazy template-foo for generating IDs to identify
options
2) I’ve moved all the new API components out of the cl namespace
3) There is no option storage outside the OptionRegistry, but I have defined a
get API and setup template methods for reading from a store and suggested API
for LLVMContext to implement
4) I’ve defined a new API for defining options in the form of the opt_builder
object. This is intended to be a transitional API only, but it allows us to
migrate off the cl::opt template-foo

As with my previous patches, these patches are designed to work with existing
cl::opts.

Thanks,
-Chris

> On Aug 26, 2014, at 6:18 PM, Robinson, Paul <Paul_Robinson at
playstation.sony.com> wrote:
> 
> I count 15 occurrences of "-backend-option" in
clang/lib/Driver/Tools.cpp and most (all? didn't look at them real hard) of
those aren't debugging options.  Presumably those can all be solved by
adding more plumbing, but it would be *really* nice to make that plumbing easier
to do.
> --paulr
>   <>
> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at
cs.uiuc.edu] On Behalf Of Chandler Carruth
> Sent: Tuesday, August 26, 2014 10:42 AM
> To: Chris Bieneman
> Cc: LLVM Dev
> Subject: Re: [LLVMdev] [RFC] Removing static initializers for command line
options
>  
> A bunch of IRC discussions etc have taken place, and I wanted to surface
some of the points from them here. Also, I wanted to give a more fully
thought-through set of my own perspectives.
>  
>  
> First, I'd like to re-state the goals as I think those have gotten
muddied.
>  
> 1) Remove most of LLVM's static initializers (that is, dynamic
initialization code for static storage duration objects) by moving away from
global objects with non-trivial constructors of 'llvm::cl::opt' types in
the LLVM *libraries* (they remain fine in the tools of course).
>  
> 2) Support separate LLVM users in a single address space having different
values for the debugging knobs currently managed through the
'llvm::cl::opt' command line flags mechanism.
>  
> 3) (related to #2) Support some library API for setting the debugging knobs
currently managed through the 'llvm::cl::opt' command line flags
mechanism.
>  
>  
> Now, I want to change my position on one point because after talking to
Chris, Jim, and others I understand the problem significantly better. I am now
convinced that it is impossible to fix even #1 without making a change for every
single option in LLVM. Why? Because we have to *register* all of the flags, and
this registration inherently cannot be done from a global's constructor
without introducing the undesirable static initializers[1].
>  
> I also think that in order to address #2 we would need to make some change
to essentially every option in LLVM as we would have to connect it to non-global
storage.
>  
> Given that, I think it does make sense to try to address #1 and #2 at the
same time -- trying to limit the change to just one or the other would very
likely result in a significantly different API for creating, registering, and
using these options, and I would rather not make all of the LLVM developers
re-learn how to do this 2 times if we can instead do it 1 time.
>  
>  
> However, doing #2 only really makes sense if we also do #3 to let people
*use* this new freedom. =] Owen convinced me that there really is a good reason
to support #3 as part of the LLVM libraries, however I wanted to raise my
primary concern about this on the mailing list because it hasn't really been
discussed.
>  
> These "debugging knobs" or library level options which
*aren't* exposed in the library's APIs are very risky to expose in a
side-channel API: they are often completely unsupported or unsupportable options
that were never intended for "production" usage. Once we expose these
options via some API to library users, we run a very serious risk of getting
locked into supporting behaviors or debugging misbehavior that were never
expected or planned for...
>  
> However, the debugging use case is real and important. So I want to propose
that we guard many of the paths to setting these options with NDEBUG so that in
non-asserts builds, library users can't override these options. This would
mean the C++ API for setting this in LLVMContext (and the C API if we ever
expose it there) would only support setting these in asserts builds. We could
even sink the checking into the registry. The name should probably have
"debug" in it somewhere. It would then have some
InternalUnsupportedScaryBad API which is not actually in the header files but is
used in LLVM's tools to maintain their current behavior without documenting
a public API. It's not bulletproof, but it seemed that it would be
sufficient for the debugging use cases and is a strong statement of
"unsupported" IMO.
>  
>  
> The only other observation I would like to make is that if we're going
to do this, we really should take the time to try to sort out a really *good*
API. Hopefully we'll be slightly less wrong this time, and maybe the next
transition can be more incremental.
>  
>  
> Ok, on to the more low-level parts of this email, here some specific
thoughts for how to design the API for further discussion:
>  
> - I really like the idea of using a 'void*' derived from some
static tag as a way to identify uniquely each option. I also think we can use
some minimal "reflection" techniques to significantly simplify this.
>  
> - The need for a global registry is... distasteful, but I agree inevitable.
>  
> - I'd like to avoid a separate "option store". My current
suggestion is to use the LLVMContext, and to add a context to the layers which
don't have an LLVMContext and where these kinds of options make sense.
>  
> - I would actually use a simple 'void*' -> string mapping in the
"store" of the context. (Maybe we can actually use StringMap?) I would
still provide the type to the registry and use that to validate options when
parsing. When getting the value out of the "store" in the context, we
can parse it into the data-type and assert fail on parse errors. I don't
feel strongly about this, but without "std::any" it seems simpler than
rolling our own with a void* or a union.
>  
> - I would provide a simple API on the LLVMContext to get an
"option" string for a given key. This API could be exposed by any
context, and we can write the option value extraction library to accept a
generic T which supports such a query method. That way there is *very* loose
coupling between these and it is easy to support any context object at any
layer.
>  
> - The registry should essentially provide an interface to parse an argv
string list into such a map from 'void*' -> string. The LLVMContext
(or any other context) can then be populated by the result of this parse. Each
library can parse whatever argv it wants.
>  
> - I don't think we should keep piling on the interface design of the
existing cl::opt stuff. I would make a clean break to a new header / library,
and just build some code to map these into the cl::opt parsing code from the
registry (we already do stuff like this for pass names). This will also help the
two systems co-exist but stay separate during the (inevitably long) transition
period.
>  
> - As a specific example of what I would change from the cl::opt stuff: I
think we should make the required arguments just be required and positional, and
the optional arguments use a fluent-style interface (yea, I know, but I think it
works well here) rather than the weird variadic-ish thing...
>  
>  
> Once there is a patch that seems like a reasonable start at this, I'd
actually like to get the new library added and just one pass (the one you're
using seems great) converted. I'd then like to try a few experiments to see
if we can improve some of the nitty-gritty details of the API before we roll it
out widely.
>  
> Seem reasonable? I'm actually happy to help with some of the API
experiments and the rollout here.
> -Chandler
>  
>  
> [1]: I'd actually like to understand why on some platforms the explicit
global initialization routines are so much better than global constructors...
But that's probably a topic for on off-list or IRC conversation. I don't
want to derail this thread with that.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140827/3daea8d9/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cl_opt-revised.diff
Type: application/octet-stream
Size: 11563 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140827/3daea8d9/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140827/3daea8d9/attachment-0001.html>

On Wed, Aug 27, 2014 at 2:36 PM, Chris Bieneman <beanz at apple.com>
wrote:
> Chandler brought up a lot of really great points. One of his big points
> was that if we’re going to touch every pass, we may as well fix the cl::opt
> API too.
>
> Toward that end I’ve put together some patches with a new fluent-style API
> as a starting point of a conversation.
>
> Disclaimer: To make these patches work with the existing cl::opts there is
> some nasty hackery going on in CommandLine.h — For the sake of this
> conversation I’d like to focus our attention on the API for creating and
> accessing options in Scalarizer.cpp
>
Yep, I'll actually only use that part of the patch to comment on, I agree
the rest are largely details.

>
> My intent with these changes is to provide a groundwork for meeting all of
> Chandler’s main points. These patches have the following high-level changes
> from my previous patches:
>
> 1) I’ve switched to some crazy template-foo for generating IDs to identify
> options
> 2) I’ve moved all the new API components out of the cl namespace
> 3) There is no option storage outside the OptionRegistry, but I have
> defined a get API and setup template methods for reading from a store and
> suggested API for LLVMContext to implement
> 4) I’ve defined a new API for defining options in the form of the
> opt_builder object. This is intended to be a transitional API only, but it
> allows us to migrate off the cl::opt template-foo
>
> As with my previous patches, these patches are designed to work with
> existing cl::opts.
>
One very high-level comment about this interaction...

I think it would be nice to end up with these debugging options fully
separated from the 'cl::opt' stuff from the perspective of code
registering
and querying an option. What I'm thinking is that we should be able to give
simple guidance within LLVM: library code doesn't use CommandLine.h, it
uses DebugOptions.h (or whatever its called).

However, as you say, we obviously need to integrate cleanly with the
cl::opt world that exists. =] I would probably structure it such that these
debugging options didn't depend on any of the details of the cl::opt
infrastructure, and instead the cl::opt stuff queried these debugging
options to collect their names and parse them when parsing command line
options. I suspect long term the library interface for setting and toggling
these may well be unrelated to the cl::opt interface; it would at least be
nice to leave that option open, which I why I would suggest layering it in
that direction. Does that make sense? Perhaps there are implementation
reasons that preclude it, I've not checked as I agree that the current code
there isn't important, but I wanted to mention the layering thing just so
it was on your radar.

Now, on to the code! =]

diff --git a/lib/Transforms/Scalar/Scalarizer.cpp
b/lib/Transforms/Scalar/Scalarizer.cpp
index 813041a..d830a48 100644
--- a/lib/Transforms/Scalar/Scalarizer.cpp
+++ b/lib/Transforms/Scalar/Scalarizer.cpp
@@ -127,9 +127,22 @@ class Scalarizer : public FunctionPass,
 public:
   static char ID;

+  template<typename OptStore>
+  void init(const OptStore &OS) {
+    initializeScalarizerPass(*PassRegistry::getPassRegistry());
+    ScalarizeLoadStore = OS.template
+      get<bool, Scalarizer, &Scalarizer::ScalarizeLoadStore>();
+  }
+
   Scalarizer() :
     FunctionPass(ID) {
-    initializeScalarizerPass(*PassRegistry::getPassRegistry());
+    init(OptionRegistry::instance());
+  }
+
+  template<typename OptStore>
+  Scalarizer(const OptStore &OS) :
+    FunctionPass(ID) {
+    init(OS);

What do you think about my suggestion to use doInitialization(Module &) and
getting the options from LLVMContext?

I understand that some layers of LLVM will need to use something other than
LLVMContext, but as mentioned in IRC, I'm interested in just having one
object that gathers common state for a layer and you can introduce new
types/objects for layers that shouldn't be querying LLVMContext.


@@ -150,6 +163,18 @@ public:
   bool visitLoadInst(LoadInst &);
   bool visitStoreInst(StoreInst &);

+  static void RegisterOptions() {
+    // This is disabled by default because having separate loads and
stores makes
+    // it more likely that the -combiner-alias-analysis limits will be
reached.
+    OptionRegistry::CreateOption<bool,
+                                Scalarizer,
+                                &Scalarizer::ScalarizeLoadStore>()
+      .setInitialValue(false)
+      .setArgStr("scalarize-load-store")
+      .setHiddenFlag(cl::Hidden)
+      .setDescription("Allow the scalarizer pass to scalarize loads and
store");

As this is a new interface, I would follow the new naming conventions here.

Also, I don't think it being a static method is really useful. What about
just making this a free function "registerOption"?


I wouldn't set the initial value here -- I think it is more clear and more
flexible to pass that into the "get" API.


The argument string isn't optional, right? The initial idea I was thinking
of for a fluent-style API was to have the required parameters as normal
ones, and the fluent API for optional. I had also imagined (but I'm not
really attached to it, curious what you think here) using it as an
option-struct builder for an optional parameter... but I've no idea what to
call it. Here is a rough example of what I'm thinking just so that we can
both see it:


  static void registerOptions() {
    registerOption<bool, Scalarizer, &Scalarizer::ScalarizeLoadStore>(
        "scalarize-load-store",
        OptionOptions
            .hidden()
            .description("Allow the scalarizer pass to scalarize loads and
stores"));
  }


However, now that I write all this, I think I was just really wrong about
the fluent API. It's not that the fluent API isn't a good way to
implement
something like what cl::opt is providing, its just that seeing this example
makes me think it's doing the wrong thing: these shouldn't be optional
flags at all.

1) The initial value *should* be optional, but that already can be optional
if its in the "get" API, and we can report errors nicely when the
option is
missing.

2) The name of the option is pretty clearly something that should be
required I think.

3) Making the description optional seems like a mistake in retrospect.
Essentially all of them have the description, and *especially* the ones
that this API is designed to replace: highly specialized options for
configuring the innards of some part of the library.

4) I think the "hidden" distinction is no longer needed. All of the
options
using this new API should be "hidden" options -- they're just
debugging
tuning knobs. It's the options that continue to use cl::opt from inside of
the tools themselves that would want to be non-hidden I think?

At that point, I think this becomes something pleasingly simple:

  static void registerOptions() {
    registerOption<bool, Scalarizer, &Scalarizer::ScalarizeLoadStore>(
        "scalarize-load-store",
        "Allow the scalarizer pass to scalarize loads and stores");
  }

Thoughts?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140829/f5c16d99/attachment.html>

On Aug 29, 2014, at 1:50 AM, Chandler Carruth <chandlerc at google.com>
wrote:
> 
> On Wed, Aug 27, 2014 at 2:36 PM, Chris Bieneman <beanz at apple.com>
wrote:
> Chandler brought up a lot of really great points. One of his big points was
that if we’re going to touch every pass, we may as well fix the cl::opt API too.
> 
> Toward that end I’ve put together some patches with a new fluent-style API
as a starting point of a conversation.
> 
> Disclaimer: To make these patches work with the existing cl::opts there is
some nasty hackery going on in CommandLine.h — For the sake of this conversation
I’d like to focus our attention on the API for creating and accessing options in
Scalarizer.cpp
> 
> Yep, I'll actually only use that part of the patch to comment on, I
agree the rest are largely details.
>  
> 
> My intent with these changes is to provide a groundwork for meeting all of
Chandler’s main points. These patches have the following high-level changes from
my previous patches:
> 
> 1) I’ve switched to some crazy template-foo for generating IDs to identify
options
> 2) I’ve moved all the new API components out of the cl namespace
> 3) There is no option storage outside the OptionRegistry, but I have
defined a get API and setup template methods for reading from a store and
suggested API for LLVMContext to implement
> 4) I’ve defined a new API for defining options in the form of the
opt_builder object. This is intended to be a transitional API only, but it
allows us to migrate off the cl::opt template-foo
> 
> As with my previous patches, these patches are designed to work with
existing cl::opts.
> 
> One very high-level comment about this interaction...
> 
> I think it would be nice to end up with these debugging options fully
separated from the 'cl::opt' stuff from the perspective of code
registering and querying an option. What I'm thinking is that we should be
able to give simple guidance within LLVM: library code doesn't use
CommandLine.h, it uses DebugOptions.h (or whatever its called).
> 
> However, as you say, we obviously need to integrate cleanly with the
cl::opt world that exists. =] I would probably structure it such that these
debugging options didn't depend on any of the details of the cl::opt
infrastructure, and instead the cl::opt stuff queried these debugging options to
collect their names and parse them when parsing command line options. I suspect
long term the library interface for setting and toggling these may well be
unrelated to the cl::opt interface; it would at least be nice to leave that
option open, which I why I would suggest layering it in that direction. Does
that make sense? Perhaps there are implementation reasons that preclude it,
I've not checked as I agree that the current code there isn't important,
but I wanted to mention the layering thing just so it was on your radar.
That makes sense. I’ll think about how to best re-structure the code and propose
some patches.
> 
> Now, on to the code! =]
> 
> diff --git a/lib/Transforms/Scalar/Scalarizer.cpp
b/lib/Transforms/Scalar/Scalarizer.cpp
> index 813041a..d830a48 100644
> --- a/lib/Transforms/Scalar/Scalarizer.cpp
> +++ b/lib/Transforms/Scalar/Scalarizer.cpp
> @@ -127,9 +127,22 @@ class Scalarizer : public FunctionPass,
>  public:
>    static char ID;
>  
> +  template<typename OptStore>
> +  void init(const OptStore &OS) {
> +    initializeScalarizerPass(*PassRegistry::getPassRegistry());
> +    ScalarizeLoadStore = OS.template
> +      get<bool, Scalarizer, &Scalarizer::ScalarizeLoadStore>();
> +  }
> +
>    Scalarizer() :
>      FunctionPass(ID) {
> -    initializeScalarizerPass(*PassRegistry::getPassRegistry());
> +    init(OptionRegistry::instance());
> +  }
> +
> +  template<typename OptStore>
> +  Scalarizer(const OptStore &OS) :
> +    FunctionPass(ID) {
> +    init(OS);
> 
> What do you think about my suggestion to use doInitialization(Module &)
and getting the options from LLVMContext?
We can go down that route. I would prefer to phase that change. If we make
doInitialization a template (like above with init), we can have it initially
called with the OptionRegistry, and replace it with the context later and not
need to revisit the pass code. I do think that whatever object we pass into
doInitialization, it should conform to the interface we decide on for retrieving
options.
> 
> I understand that some layers of LLVM will need to use something other than
LLVMContext, but as mentioned in IRC, I'm interested in just having one
object that gathers common state for a layer and you can introduce new
types/objects for layers that shouldn't be querying LLVMContext.
> 
> 
> @@ -150,6 +163,18 @@ public:
>    bool visitLoadInst(LoadInst &);
>    bool visitStoreInst(StoreInst &);
>  
> +  static void RegisterOptions() {
> +    // This is disabled by default because having separate loads and
stores makes
> +    // it more likely that the -combiner-alias-analysis limits will be
reached.
> +    OptionRegistry::CreateOption<bool,
> +                                Scalarizer,
> +                                &Scalarizer::ScalarizeLoadStore>()
> +      .setInitialValue(false)
> +      .setArgStr("scalarize-load-store")
> +      .setHiddenFlag(cl::Hidden)
> +      .setDescription("Allow the scalarizer pass to scalarize loads
and store");
> 
> As this is a new interface, I would follow the new naming conventions here.
> 
> Also, I don't think it being a static method is really useful. What
about just making this a free function "registerOption”?
If Scalarizer::ScalarizeLoadStore is a private member (and it kinda should be),
then you need this to be a static method on the class otherwise the code won’t
compile. That’s why I did it this way instead of as a standalone function.
> 
> 
> I wouldn't set the initial value here -- I think it is more clear and
more flexible to pass that into the "get" API.
I preserved the initial value where it is based on a conversation with Jim. I’m
not particularly opinionated either way. I’ll ask Jim to chime in if he feels
strongly about this.
> 
> 
> The argument string isn't optional, right? The initial idea I was
thinking of for a fluent-style API was to have the required parameters as normal
ones, and the fluent API for optional. I had also imagined (but I'm not
really attached to it, curious what you think here) using it as an option-struct
builder for an optional parameter... but I've no idea what to call it. Here
is a rough example of what I'm thinking just so that we can both see it:
> 
> 
>   static void registerOptions() {
>     registerOption<bool, Scalarizer,
&Scalarizer::ScalarizeLoadStore>(
>         "scalarize-load-store",
>         OptionOptions
>             .hidden()
>             .description("Allow the scalarizer pass to scalarize loads
and stores"));
>   }
> 
> 
> However, now that I write all this, I think I was just really wrong about
the fluent API. It's not that the fluent API isn't a good way to
implement something like what cl::opt is providing, its just that seeing this
example makes me think it's doing the wrong thing: these shouldn't be
optional flags at all.
> 
> 1) The initial value *should* be optional, but that already can be optional
if its in the "get" API, and we can report errors nicely when the
option is missing.
> 
> 2) The name of the option is pretty clearly something that should be
required I think.
> 
> 3) Making the description optional seems like a mistake in retrospect.
Essentially all of them have the description, and *especially* the ones that
this API is designed to replace: highly specialized options for configuring the
innards of some part of the library.
> 
> 4) I think the "hidden" distinction is no longer needed. All of
the options using this new API should be "hidden" options --
they're just debugging tuning knobs. It's the options that continue to
use cl::opt from inside of the tools themselves that would want to be non-hidden
I think?
> 
> At that point, I think this becomes something pleasingly simple:
> 
>   static void registerOptions() {
>     registerOption<bool, Scalarizer,
&Scalarizer::ScalarizeLoadStore>(
>         "scalarize-load-store",
>         "Allow the scalarizer pass to scalarize loads and
stores");
>   }
> 
> Thoughts?
Oddly enough the argument string IS optional, but you must have either an
argument string or cl::Positional (but not both). It is probably reasonable to
define two registerOption() calls one that takes an argument string and
description, and one that only takes a description and implies cl::Positional.

I also think we still need a fluent-style API for toggling some options that
aren’t always default values (like hidden). In the case of hidden, maybe the
better way to handle it is to make registerOption default to creating hidden
options and have a “.visible()” API for toggling it?

It seems to me like you’re suggesting an end solution where cl::opt still exists
for tool-specific options, and the register/get API exists for all of the other
options littered around the compiler. While I’m not going to object strongly to
this idea because it does serve my purposes (getting rid of static initializers
in the libraries), I don’t particularly care for this. I think fundamentally if
we are aiming to provide a better API for defining options, I don’t think there
is any reason not to use that API everywhere and to abandon the existing one
completely. If you disagree, please let me know.

-Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140829/ee23ab90/attachment.html>

Now that I’ve had my coffee and played with code a bit…

I think your idea of using doInitialization and fetching the context off the
module makes sense.

Here are some diffs to review. I still haven’t moved the default off the
registration, but I have adjusted the API a bit.

Again, please focus on Scalarizer.cpp as there is a growing amount of nastiness
outside the pass to make this all work.

Thanks,
-Chris

> On Aug 29, 2014, at 8:53 AM, Chris Bieneman <cbieneman at apple.com>
wrote:
> 
> 
> On Aug 29, 2014, at 1:50 AM, Chandler Carruth <chandlerc at google.com
<mailto:chandlerc at google.com>> wrote:
> 
>> 
>> On Wed, Aug 27, 2014 at 2:36 PM, Chris Bieneman <beanz at apple.com
<mailto:beanz at apple.com>> wrote:
>> Chandler brought up a lot of really great points. One of his big points
was that if we’re going to touch every pass, we may as well fix the cl::opt API
too.
>> 
>> Toward that end I’ve put together some patches with a new fluent-style
API as a starting point of a conversation.
>> 
>> Disclaimer: To make these patches work with the existing cl::opts there
is some nasty hackery going on in CommandLine.h — For the sake of this
conversation I’d like to focus our attention on the API for creating and
accessing options in Scalarizer.cpp
>> 
>> Yep, I'll actually only use that part of the patch to comment on, I
agree the rest are largely details.
>>  
>> 
>> My intent with these changes is to provide a groundwork for meeting all
of Chandler’s main points. These patches have the following high-level changes
from my previous patches:
>> 
>> 1) I’ve switched to some crazy template-foo for generating IDs to
identify options
>> 2) I’ve moved all the new API components out of the cl namespace
>> 3) There is no option storage outside the OptionRegistry, but I have
defined a get API and setup template methods for reading from a store and
suggested API for LLVMContext to implement
>> 4) I’ve defined a new API for defining options in the form of the
opt_builder object. This is intended to be a transitional API only, but it
allows us to migrate off the cl::opt template-foo
>> 
>> As with my previous patches, these patches are designed to work with
existing cl::opts.
>> 
>> One very high-level comment about this interaction...
>> 
>> I think it would be nice to end up with these debugging options fully
separated from the 'cl::opt' stuff from the perspective of code
registering and querying an option. What I'm thinking is that we should be
able to give simple guidance within LLVM: library code doesn't use
CommandLine.h, it uses DebugOptions.h (or whatever its called).
>> 
>> However, as you say, we obviously need to integrate cleanly with the
cl::opt world that exists. =] I would probably structure it such that these
debugging options didn't depend on any of the details of the cl::opt
infrastructure, and instead the cl::opt stuff queried these debugging options to
collect their names and parse them when parsing command line options. I suspect
long term the library interface for setting and toggling these may well be
unrelated to the cl::opt interface; it would at least be nice to leave that
option open, which I why I would suggest layering it in that direction. Does
that make sense? Perhaps there are implementation reasons that preclude it,
I've not checked as I agree that the current code there isn't important,
but I wanted to mention the layering thing just so it was on your radar.
> 
> That makes sense. I’ll think about how to best re-structure the code and
propose some patches.
> 
>> 
>> Now, on to the code! =]
>> 
>> diff --git a/lib/Transforms/Scalar/Scalarizer.cpp
b/lib/Transforms/Scalar/Scalarizer.cpp
>> index 813041a..d830a48 100644
>> --- a/lib/Transforms/Scalar/Scalarizer.cpp
>> +++ b/lib/Transforms/Scalar/Scalarizer.cpp
>> @@ -127,9 +127,22 @@ class Scalarizer : public FunctionPass,
>>  public:
>>    static char ID;
>>  
>> +  template<typename OptStore>
>> +  void init(const OptStore &OS) {
>> +    initializeScalarizerPass(*PassRegistry::getPassRegistry());
>> +    ScalarizeLoadStore = OS.template
>> +      get<bool, Scalarizer,
&Scalarizer::ScalarizeLoadStore>();
>> +  }
>> +
>>    Scalarizer() :
>>      FunctionPass(ID) {
>> -    initializeScalarizerPass(*PassRegistry::getPassRegistry());
>> +    init(OptionRegistry::instance());
>> +  }
>> +
>> +  template<typename OptStore>
>> +  Scalarizer(const OptStore &OS) :
>> +    FunctionPass(ID) {
>> +    init(OS);
>> 
>> What do you think about my suggestion to use doInitialization(Module
&) and getting the options from LLVMContext?
> 
> We can go down that route. I would prefer to phase that change. If we make
doInitialization a template (like above with init), we can have it initially
called with the OptionRegistry, and replace it with the context later and not
need to revisit the pass code. I do think that whatever object we pass into
doInitialization, it should conform to the interface we decide on for retrieving
options.
> 
>> 
>> I understand that some layers of LLVM will need to use something other
than LLVMContext, but as mentioned in IRC, I'm interested in just having one
object that gathers common state for a layer and you can introduce new
types/objects for layers that shouldn't be querying LLVMContext.
>> 
>> 
>> @@ -150,6 +163,18 @@ public:
>>    bool visitLoadInst(LoadInst &);
>>    bool visitStoreInst(StoreInst &);
>>  
>> +  static void RegisterOptions() {
>> +    // This is disabled by default because having separate loads and
stores makes
>> +    // it more likely that the -combiner-alias-analysis limits will be
reached.
>> +    OptionRegistry::CreateOption<bool,
>> +                                Scalarizer,
>> +                               
&Scalarizer::ScalarizeLoadStore>()
>> +      .setInitialValue(false)
>> +      .setArgStr("scalarize-load-store")
>> +      .setHiddenFlag(cl::Hidden)
>> +      .setDescription("Allow the scalarizer pass to scalarize
loads and store");
>> 
>> As this is a new interface, I would follow the new naming conventions
here.
>> 
>> Also, I don't think it being a static method is really useful. What
about just making this a free function "registerOption”?
> 
> If Scalarizer::ScalarizeLoadStore is a private member (and it kinda should
be), then you need this to be a static method on the class otherwise the code
won’t compile. That’s why I did it this way instead of as a standalone function.
> 
>> 
>> 
>> I wouldn't set the initial value here -- I think it is more clear
and more flexible to pass that into the "get" API.
> 
> I preserved the initial value where it is based on a conversation with Jim.
I’m not particularly opinionated either way. I’ll ask Jim to chime in if he
feels strongly about this.
> 
>> 
>> 
>> The argument string isn't optional, right? The initial idea I was
thinking of for a fluent-style API was to have the required parameters as normal
ones, and the fluent API for optional. I had also imagined (but I'm not
really attached to it, curious what you think here) using it as an option-struct
builder for an optional parameter... but I've no idea what to call it. Here
is a rough example of what I'm thinking just so that we can both see it:
>> 
>> 
>>   static void registerOptions() {
>>     registerOption<bool, Scalarizer,
&Scalarizer::ScalarizeLoadStore>(
>>         "scalarize-load-store",
>>         OptionOptions
>>             .hidden()
>>             .description("Allow the scalarizer pass to scalarize
loads and stores"));
>>   }
>> 
>> 
>> However, now that I write all this, I think I was just really wrong
about the fluent API. It's not that the fluent API isn't a good way to
implement something like what cl::opt is providing, its just that seeing this
example makes me think it's doing the wrong thing: these shouldn't be
optional flags at all.
>> 
>> 1) The initial value *should* be optional, but that already can be
optional if its in the "get" API, and we can report errors nicely when
the option is missing.
>> 
>> 2) The name of the option is pretty clearly something that should be
required I think.
>> 
>> 3) Making the description optional seems like a mistake in retrospect.
Essentially all of them have the description, and *especially* the ones that
this API is designed to replace: highly specialized options for configuring the
innards of some part of the library.
>> 
>> 4) I think the "hidden" distinction is no longer needed. All
of the options using this new API should be "hidden" options --
they're just debugging tuning knobs. It's the options that continue to
use cl::opt from inside of the tools themselves that would want to be non-hidden
I think?
>> 
>> At that point, I think this becomes something pleasingly simple:
>> 
>>   static void registerOptions() {
>>     registerOption<bool, Scalarizer,
&Scalarizer::ScalarizeLoadStore>(
>>         "scalarize-load-store",
>>         "Allow the scalarizer pass to scalarize loads and
stores");
>>   }
>> 
>> Thoughts?
> 
> Oddly enough the argument string IS optional, but you must have either an
argument string or cl::Positional (but not both). It is probably reasonable to
define two registerOption() calls one that takes an argument string and
description, and one that only takes a description and implies cl::Positional.
> 
> I also think we still need a fluent-style API for toggling some options
that aren’t always default values (like hidden). In the case of hidden, maybe
the better way to handle it is to make registerOption default to creating hidden
options and have a “.visible()” API for toggling it?
> 
> It seems to me like you’re suggesting an end solution where cl::opt still
exists for tool-specific options, and the register/get API exists for all of the
other options littered around the compiler. While I’m not going to object
strongly to this idea because it does serve my purposes (getting rid of static
initializers in the libraries), I don’t particularly care for this. I think
fundamentally if we are aiming to provide a better API for defining options, I
don’t think there is any reason not to use that API everywhere and to abandon
the existing one completely. If you disagree, please let me know.
> 
> -Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140829/c84c1024/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cl_opt-revised.diff
Type: application/octet-stream
Size: 14914 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140829/c84c1024/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140829/c84c1024/attachment-0001.html>

> On Aug 29, 2014, at 8:53 AM, Chris Bieneman <cbieneman at apple.com>
wrote:
> 
> 
> On Aug 29, 2014, at 1:50 AM, Chandler Carruth <chandlerc at
google.com> wrote:
> 
>> 
>> On Wed, Aug 27, 2014 at 2:36 PM, Chris Bieneman <beanz at
apple.com> wrote:
>> Chandler brought up a lot of really great points. One of his big points
was that if we’re going to touch every pass, we may as well fix the cl::opt API
too.
>> 
>> Toward that end I’ve put together some patches with a new fluent-style
API as a starting point of a conversation.
>> 
>> Disclaimer: To make these patches work with the existing cl::opts there
is some nasty hackery going on in CommandLine.h — For the sake of this
conversation I’d like to focus our attention on the API for creating and
accessing options in Scalarizer.cpp
>> 
>> Yep, I'll actually only use that part of the patch to comment on, I
agree the rest are largely details.
>>  
>> 
>> My intent with these changes is to provide a groundwork for meeting all
of Chandler’s main points. These patches have the following high-level changes
from my previous patches:
>> 
>> 1) I’ve switched to some crazy template-foo for generating IDs to
identify options
>> 2) I’ve moved all the new API components out of the cl namespace
>> 3) There is no option storage outside the OptionRegistry, but I have
defined a get API and setup template methods for reading from a store and
suggested API for LLVMContext to implement
>> 4) I’ve defined a new API for defining options in the form of the
opt_builder object. This is intended to be a transitional API only, but it
allows us to migrate off the cl::opt template-foo
>> 
>> As with my previous patches, these patches are designed to work with
existing cl::opts.
>> 
>> One very high-level comment about this interaction...
>> 
>> I think it would be nice to end up with these debugging options fully
separated from the 'cl::opt' stuff from the perspective of code
registering and querying an option. What I'm thinking is that we should be
able to give simple guidance within LLVM: library code doesn't use
CommandLine.h, it uses DebugOptions.h (or whatever its called).
>> 
>> However, as you say, we obviously need to integrate cleanly with the
cl::opt world that exists. =] I would probably structure it such that these
debugging options didn't depend on any of the details of the cl::opt
infrastructure, and instead the cl::opt stuff queried these debugging options to
collect their names and parse them when parsing command line options. I suspect
long term the library interface for setting and toggling these may well be
unrelated to the cl::opt interface; it would at least be nice to leave that
option open, which I why I would suggest layering it in that direction. Does
that make sense? Perhaps there are implementation reasons that preclude it,
I've not checked as I agree that the current code there isn't important,
but I wanted to mention the layering thing just so it was on your radar.
> 
> That makes sense. I’ll think about how to best re-structure the code and
propose some patches.
> 
>> 
>> Now, on to the code! =]
>> 
>> diff --git a/lib/Transforms/Scalar/Scalarizer.cpp
b/lib/Transforms/Scalar/Scalarizer.cpp
>> index 813041a..d830a48 100644
>> --- a/lib/Transforms/Scalar/Scalarizer.cpp
>> +++ b/lib/Transforms/Scalar/Scalarizer.cpp
>> @@ -127,9 +127,22 @@ class Scalarizer : public FunctionPass,
>>  public:
>>    static char ID;
>>  
>> +  template<typename OptStore>
>> +  void init(const OptStore &OS) {
>> +    initializeScalarizerPass(*PassRegistry::getPassRegistry());
>> +    ScalarizeLoadStore = OS.template
>> +      get<bool, Scalarizer,
&Scalarizer::ScalarizeLoadStore>();
>> +  }
>> +
>>    Scalarizer() :
>>      FunctionPass(ID) {
>> -    initializeScalarizerPass(*PassRegistry::getPassRegistry());
>> +    init(OptionRegistry::instance());
>> +  }
>> +
>> +  template<typename OptStore>
>> +  Scalarizer(const OptStore &OS) :
>> +    FunctionPass(ID) {
>> +    init(OS);
>> 
>> What do you think about my suggestion to use doInitialization(Module
&) and getting the options from LLVMContext?
> 
> We can go down that route. I would prefer to phase that change. If we make
doInitialization a template (like above with init), we can have it initially
called with the OptionRegistry, and replace it with the context later and not
need to revisit the pass code. I do think that whatever object we pass into
doInitialization, it should conform to the interface we decide on for retrieving
options.
> 
>> 
>> I understand that some layers of LLVM will need to use something other
than LLVMContext, but as mentioned in IRC, I'm interested in just having one
object that gathers common state for a layer and you can introduce new
types/objects for layers that shouldn't be querying LLVMContext.
>> 
>> 
>> @@ -150,6 +163,18 @@ public:
>>    bool visitLoadInst(LoadInst &);
>>    bool visitStoreInst(StoreInst &);
>>  
>> +  static void RegisterOptions() {
>> +    // This is disabled by default because having separate loads and
stores makes
>> +    // it more likely that the -combiner-alias-analysis limits will be
reached.
>> +    OptionRegistry::CreateOption<bool,
>> +                                Scalarizer,
>> +                               
&Scalarizer::ScalarizeLoadStore>()
>> +      .setInitialValue(false)
>> +      .setArgStr("scalarize-load-store")
>> +      .setHiddenFlag(cl::Hidden)
>> +      .setDescription("Allow the scalarizer pass to scalarize
loads and store");
>> 
>> As this is a new interface, I would follow the new naming conventions
here.
>> 
>> Also, I don't think it being a static method is really useful. What
about just making this a free function "registerOption”?
> 
> If Scalarizer::ScalarizeLoadStore is a private member (and it kinda should
be), then you need this to be a static method on the class otherwise the code
won’t compile. That’s why I did it this way instead of as a standalone function.
> 
>> 
>> 
>> I wouldn't set the initial value here -- I think it is more clear
and more flexible to pass that into the "get" API.
> 
> I preserved the initial value where it is based on a conversation with Jim.
I’m not particularly opinionated either way. I’ll ask Jim to chime in if he
feels strongly about this.
It depends on the specifics, but in general having the default value for the
options be specified (potentially differently) on every call to the getter
method feels very wrong to me. Perhaps I am misunderstanding the proposal?
> 
>> 
>> 
>> The argument string isn't optional, right? The initial idea I was
thinking of for a fluent-style API was to have the required parameters as normal
ones, and the fluent API for optional. I had also imagined (but I'm not
really attached to it, curious what you think here) using it as an option-struct
builder for an optional parameter... but I've no idea what to call it. Here
is a rough example of what I'm thinking just so that we can both see it:
>> 
>> 
>>   static void registerOptions() {
>>     registerOption<bool, Scalarizer,
&Scalarizer::ScalarizeLoadStore>(
>>         "scalarize-load-store",
>>         OptionOptions
>>             .hidden()
>>             .description("Allow the scalarizer pass to scalarize
loads and stores"));
>>   }
>> 
>> 
>> However, now that I write all this, I think I was just really wrong
about the fluent API. It's not that the fluent API isn't a good way to
implement something like what cl::opt is providing, its just that seeing this
example makes me think it's doing the wrong thing: these shouldn't be
optional flags at all.
>> 
>> 1) The initial value *should* be optional, but that already can be
optional if its in the "get" API, and we can report errors nicely when
the option is missing.
>> 
>> 2) The name of the option is pretty clearly something that should be
required I think.
>> 
>> 3) Making the description optional seems like a mistake in retrospect.
Essentially all of them have the description, and *especially* the ones that
this API is designed to replace: highly specialized options for configuring the
innards of some part of the library.
>> 
>> 4) I think the "hidden" distinction is no longer needed. All
of the options using this new API should be "hidden" options --
they're just debugging tuning knobs. It's the options that continue to
use cl::opt from inside of the tools themselves that would want to be non-hidden
I think?
>> 
>> At that point, I think this becomes something pleasingly simple:
>> 
>>   static void registerOptions() {
>>     registerOption<bool, Scalarizer,
&Scalarizer::ScalarizeLoadStore>(
>>         "scalarize-load-store",
>>         "Allow the scalarizer pass to scalarize loads and
stores");
>>   }
>> 
>> Thoughts?
> 
> Oddly enough the argument string IS optional, but you must have either an
argument string or cl::Positional (but not both). It is probably reasonable to
define two registerOption() calls one that takes an argument string and
description, and one that only takes a description and implies cl::Positional.
> 
> I also think we still need a fluent-style API for toggling some options
that aren’t always default values (like hidden). In the case of hidden, maybe
the better way to handle it is to make registerOption default to creating hidden
options and have a “.visible()” API for toggling it?
> 
> It seems to me like you’re suggesting an end solution where cl::opt still
exists for tool-specific options, and the register/get API exists for all of the
other options littered around the compiler. While I’m not going to object
strongly to this idea because it does serve my purposes (getting rid of static
initializers in the libraries), I don’t particularly care for this. I think
fundamentally if we are aiming to provide a better API for defining options, I
don’t think there is any reason not to use that API everywhere and to abandon
the existing one completely. If you disagree, please let me know.
> 
> -Chris
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140829/a0bfff54/attachment.html>

On Fri, Aug 29, 2014 at 11:29 AM, Jim Grosbach <grosbach at apple.com>
wrote:
> It depends on the specifics, but in general having the default value for
> the options be specified (potentially differently) on every call to the
> getter method feels very wrong to me. Perhaps I am misunderstanding the
> proposal?

No, I think you're understanding the proposal.

I agree that the default changing is... ew.

However, the more I think about it, the less sense ever *not* having a
default makes for these options. So maybe it would be fine to just always
require an initial value when registering?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140829/1cc135cc/attachment.html>

> On Aug 29, 2014, at 12:47 PM, Chandler Carruth <chandlerc at
google.com> wrote:
> 
> 
> On Fri, Aug 29, 2014 at 11:29 AM, Jim Grosbach <grosbach at
apple.com> wrote:
> It depends on the specifics, but in general having the default value for
the options be specified (potentially differently) on every call to the getter
method feels very wrong to me. Perhaps I am misunderstanding the proposal?
> 
> No, I think you're understanding the proposal.
> 
> I agree that the default changing is... ew.
> 
> However, the more I think about it, the less sense ever *not* having a
default makes for these options. So maybe it would be fine to just always
require an initial value when registering?
Yeah, I think that’s the right way to go here.

-Jim
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140829/f0a921d1/attachment.html>

So, trying to quickly reply to what I think may be a high-level point of
confusion that we should sift through early:

On Fri, Aug 29, 2014 at 8:53 AM, Chris Bieneman <cbieneman at apple.com>
wrote:
> Oddly enough the argument string IS optional, but you must have either an
> argument string or cl::Positional (but not both). It is probably reasonable
> to define two registerOption() calls one that takes an argument string and
> description, and one that only takes a description and implies
> cl::Positional.
>
I think all of this comes down to the point you make below:

It seems to me like you’re suggesting an end solution where cl::opt
still
> exists for tool-specific options, and the register/get API exists for all
> of the other options littered around the compiler. While I’m not going to
> object strongly to this idea because it does serve my purposes (getting rid
> of static initializers in the libraries), I don’t particularly care for
> this. I think fundamentally if we are aiming to provide a better API for
> defining options, I don’t think there is any reason not to use that API
> everywhere and to abandon the existing one completely. If you disagree,
> please let me know.
>
Ah, ok, here we have uncovered what I suspect is the underpinning set of
different assumptions / directions that are creating some of the confusion.
Now that you've put your finger on it (thanks!) I'll try actually
address
this. Sorry if this is a bit of a re-statement, but hopefully it'll at
least let you respond directly with where my thinking has gone off the
rails.

So I am definitely trying to design this as a system totally divorced from
the tool-specific options, or actually a command line system of any form.
That's why stuff like positional arguments and the cl::opt tie-ins
aren't
showing up in my thoughts and suggestions.  What you say makes perfect
sense if we're building a replacement for *all* of cl::opt's
functionality.
While I would be interested in trying to do that (and we already have one
replacement in tree used by Clang and LLD to parse command line options),
I'm suggesting that what the LLVM *libraries* need is something completely
separable from a command line interface, but which is easy to build a
command line interface around.

The way I'm thinking of this is as a generic collection of optional
key->value settings. The libraries don't have any business mucking with
positional parameters, non-hidden options, or the other concerns of a
command line system, and the code it uses (and the APIs it uses) to
register and use these options is a *better* interface when it is narrower
and doesn't have to support these features.

Now, I think we should still be able to expose these via the 'cl::opt'
layer in the tools that are using that layer. But we should also be able to
expose it via an API to say webkit or whomever needs it there. And we
should be able to wrap it in a nicer command line interface in Clang.


Is that a convincing argument for you to restrict the scope of this
interface? Maybe there are other things that you'd like to do here that
motivate going the other direction?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140829/d61b8433/attachment.html>

This is where I thought you might be going, and I’m glad we’re now talking on
the same page.

I think the philosophically I agree with you on most points, but I do have a few
comments and disagreements.

On Aug 29, 2014, at 1:09 PM, Chandler Carruth <chandlerc at google.com>
wrote:
> So, trying to quickly reply to what I think may be a high-level point of
confusion that we should sift through early:
> 
> On Fri, Aug 29, 2014 at 8:53 AM, Chris Bieneman <cbieneman at
apple.com> wrote:
> Oddly enough the argument string IS optional, but you must have either an
argument string or cl::Positional (but not both). It is probably reasonable to
define two registerOption() calls one that takes an argument string and
description, and one that only takes a description and implies cl::Positional.
> 
> I think all of this comes down to the point you make below: 
> 
> It seems to me like you’re suggesting an end solution where cl::opt still
exists for tool-specific options, and the register/get API exists for all of the
other options littered around the compiler. While I’m not going to object
strongly to this idea because it does serve my purposes (getting rid of static
initializers in the libraries), I don’t particularly care for this. I think
fundamentally if we are aiming to provide a better API for defining options, I
don’t think there is any reason not to use that API everywhere and to abandon
the existing one completely. If you disagree, please let me know.
> 
> Ah, ok, here we have uncovered what I suspect is the underpinning set of
different assumptions / directions that are creating some of the confusion. Now
that you've put your finger on it (thanks!) I'll try actually address
this. Sorry if this is a bit of a re-statement, but hopefully it'll at least
let you respond directly with where my thinking has gone off the rails.
> 
> So I am definitely trying to design this as a system totally divorced from
the tool-specific options, or actually a command line system of any form.
That's why stuff like positional arguments and the cl::opt tie-ins
aren't showing up in my thoughts and suggestions.  What you say makes
perfect sense if we're building a replacement for *all* of cl::opt's
functionality. While I would be interested in trying to do that (and we already
have one replacement in tree used by Clang and LLD to parse command line
options), I'm suggesting that what the LLVM *libraries* need is something
completely separable from a command line interface, but which is easy to build a
command line interface around.
I think this is absolutely correct in the general sense. When you are talking
about libraries as clients the notion of a command line just doesn’t makes sense
in all use cases, so it stands to reason you need some completely new
abstraction. But...
> 
> The way I'm thinking of this is as a generic collection of optional
key->value settings. The libraries don't have any business mucking with
positional parameters, non-hidden options, or the other concerns of a command
line system, and the code it uses (and the APIs it uses) to register and use
these options is a *better* interface when it is narrower and doesn't have
to support these features.
While it is true that we probably don’t need to allow the libraries to mess
around with positional parameters, non-hidden options are (I think) a different
story. If you look at include/llvm/CodeGen/CommandFlags.h, there are a number of
flags defined here that are not positional, not hidden, but only relevant to the
CodeGen library. It is probably reasonable that these flags be transitioned to
the new API, but they also should be exposed on the command line.

Aside from those options there are probably very few cases where it actually
makes sense for library options to not be hidden, although I must add the caveat
that I’m sure there are plenty of library options that are not hidden today and
making this change will make them hidden. I’m ok with this, but it does change
one of my original goals because it will actually modify the behavior of the
command line.
> 
> Now, I think we should still be able to expose these via the
'cl::opt' layer in the tools that are using that layer. But we should
also be able to expose it via an API to say webkit or whomever needs it there.
And we should be able to wrap it in a nicer command line interface in Clang.
I think this is a big step in the right direction for clients of LLVM libraries
that aren’t command line driven, so this would be a big win.
> 
> 
> Is that a convincing argument for you to restrict the scope of this
interface? Maybe there are other things that you'd like to do here that
motivate going the other direction?
How do you feel about also providing a fluent-style API for toggling the
(hopefully) uncommon options?

-Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140829/32a05ced/attachment.html>

On Fri, Aug 29, 2014 at 1:38 PM, Chris Bieneman <cbieneman at apple.com>
wrote:
> While it is true that we probably don’t need to allow the libraries to
> mess around with positional parameters, non-hidden options are (I think) a
> different story. If you look at include/llvm/CodeGen/CommandFlags.h, there
> are a number of flags defined here that are not positional, not hidden, but
> only relevant to the CodeGen library. It is probably reasonable that these
> flags be transitioned to the new API, but they also should be exposed on
> the command line.
>
So, the 'hidden' flags are *all* exposed on the commandline, they just
aren't in the *help* output.

My thinking (which perhaps is wrong) is that the library options of this
form probably shouldn't be in the help output. The help output should
probably talk about the tool commandline flags which drive actual API knobs
in constructors of things.

>
> Aside from those options there are probably very few cases where it
> actually makes sense for library options to not be hidden, although I must
> add the caveat that I’m sure there are plenty of library options that are
> not hidden today and making this change will make them hidden. I’m ok with
> this, but it does change one of my original goals because it will actually
> modify the behavior of the command line.
>
This should only change the behavior of '-help' which I think is fine.
> Is that a convincing argument for you to restrict the scope of this
> interface? Maybe there are other things that you'd like to do here that
> motivate going the other direction?
>
>
> How do you feel about also providing a fluent-style API for toggling the
> (hopefully) uncommon options?
>
I think that if we have optional things that don't make sense as one or two
unsurprising optional arguments, the fluent API design is the right one.

But I'd like to understand what those optional properties are. Currently,
all of the ones we've discussed seem fine to either be required, or be
completely omitted.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140829/1b5cfce7/attachment.html>

This all sounds fine to me.

How do you feel about the idea of committing changes with some cleanup from my
last patches and converting a few  cl::opts to the new API to see how the new
API feels? I can provide patches either later today or this weekend.

-Chris

On Aug 29, 2014, at 2:14 PM, Chandler Carruth <chandlerc at google.com>
wrote:
> 
> On Fri, Aug 29, 2014 at 1:38 PM, Chris Bieneman <cbieneman at
apple.com> wrote:
> While it is true that we probably don’t need to allow the libraries to mess
around with positional parameters, non-hidden options are (I think) a different
story. If you look at include/llvm/CodeGen/CommandFlags.h, there are a number of
flags defined here that are not positional, not hidden, but only relevant to the
CodeGen library. It is probably reasonable that these flags be transitioned to
the new API, but they also should be exposed on the command line.
> 
> So, the 'hidden' flags are *all* exposed on the commandline, they
just aren't in the *help* output.
> 
> My thinking (which perhaps is wrong) is that the library options of this
form probably shouldn't be in the help output. The help output should
probably talk about the tool commandline flags which drive actual API knobs in
constructors of things.
>  
> 
> Aside from those options there are probably very few cases where it
actually makes sense for library options to not be hidden, although I must add
the caveat that I’m sure there are plenty of library options that are not hidden
today and making this change will make them hidden. I’m ok with this, but it
does change one of my original goals because it will actually modify the
behavior of the command line.
> 
> This should only change the behavior of '-help' which I think is
fine.
>> Is that a convincing argument for you to restrict the scope of this
interface? Maybe there are other things that you'd like to do here that
motivate going the other direction?
> 
> How do you feel about also providing a fluent-style API for toggling the
(hopefully) uncommon options?
> 
> I think that if we have optional things that don't make sense as one or
two unsurprising optional arguments, the fluent API design is the right one.
> 
> But I'd like to understand what those optional properties are.
Currently, all of the ones we've discussed seem fine to either be required,
or be completely omitted.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140829/5b350801/attachment.html>

Picking this back up, I think that your patch with some cleanups is
probably the right starting point. I think the only really substantive
issue I currently have is that I'd rather have the OptionRegistry in the
LLVMContext rather than in a singleton method. I understand we'll also have
to add similar registries to other context objects (MCContext at least),
but I think it's useful to start off with just one context initially.

How do you want to go about the review / cleanups on the patch? Maybe a new
thread? I have a couple of significant things and probably a bunch of
really minor cleanup stuff.

While you may already have much of this fixed, just listing the things I've
seen here so I don't forget:

- I would use a DenseMap or a sorted vector for the lookups. While not
super hot, std::map is pretty terrible.
- Naming of parameters likely needs updating to match conventions...
- I think you can kill off the hidden flag from the new API and just always
mark these as hidden with cl::opt. Any flags which *aren't* marked as
hidden probably shouldn't migrate to this API and instead should become
constructor parameters controlled at API boundaries (and surfacing with
non-hidden cl::opt objects in the tools rather than the libraries).
- I think you can kill off the builder based on the above. Most of the
builder is already redundant with the register API.
- Go ahead and clang-format the CommandLine.{h,cpp} code you're going to
need to touch so that the diff is sane.

I think everything else I see will just fall out of the cleanup.

Sound like a plan?



On Fri, Aug 29, 2014 at 4:40 PM, Chris Bieneman <cbieneman at apple.com>
wrote:
> This all sounds fine to me.
>
> How do you feel about the idea of committing changes with some cleanup
> from my last patches and converting a few  cl::opts to the new API to see
> how the new API feels? I can provide patches either later today or this
> weekend.
>
> -Chris
>
> On Aug 29, 2014, at 2:14 PM, Chandler Carruth <chandlerc at
google.com>
> wrote:
>
>
> On Fri, Aug 29, 2014 at 1:38 PM, Chris Bieneman <cbieneman at
apple.com>
> wrote:
>
>> While it is true that we probably don’t need to allow the libraries to
>> mess around with positional parameters, non-hidden options are (I
think) a
>> different story. If you look at include/llvm/CodeGen/CommandFlags.h,
there
>> are a number of flags defined here that are not positional, not hidden,
but
>> only relevant to the CodeGen library. It is probably reasonable that
these
>> flags be transitioned to the new API, but they also should be exposed
on
>> the command line.
>>
>
> So, the 'hidden' flags are *all* exposed on the commandline, they
just
> aren't in the *help* output.
>
> My thinking (which perhaps is wrong) is that the library options of this
> form probably shouldn't be in the help output. The help output should
> probably talk about the tool commandline flags which drive actual API knobs
> in constructors of things.
>
>
>>
>> Aside from those options there are probably very few cases where it
>> actually makes sense for library options to not be hidden, although I
must
>> add the caveat that I’m sure there are plenty of library options that
are
>> not hidden today and making this change will make them hidden. I’m ok
with
>> this, but it does change one of my original goals because it will
actually
>> modify the behavior of the command line.
>>
>
> This should only change the behavior of '-help' which I think is
fine.
>
>> Is that a convincing argument for you to restrict the scope of this
>> interface? Maybe there are other things that you'd like to do here
that
>> motivate going the other direction?
>>
>>
>> How do you feel about also providing a fluent-style API for toggling
the
>> (hopefully) uncommon options?
>>
>
> I think that if we have optional things that don't make sense as one or
> two unsurprising optional arguments, the fluent API design is the right
one.
>
> But I'd like to understand what those optional properties are.
Currently,
> all of the ones we've discussed seem fine to either be required, or be
> completely omitted.
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140916/c2390703/attachment.html>

On Sep 16, 2014, at 5:28 PM, Chandler Carruth <chandlerc at google.com>
wrote:
> Picking this back up, I think that your patch with some cleanups is
probably the right starting point. I think the only really substantive issue I
currently have is that I'd rather have the OptionRegistry in the LLVMContext
rather than in a singleton method. I understand we'll also have to add
similar registries to other context objects (MCContext at least), but I think
it's useful to start off with just one context initially.
In my last patches I had the OptionRegistry really just being a template type
that conformed to the get interface. I was having it be a singleton so that we
have a way to gradually transition. To make it the context means we'll need
to change all the pass constructors and the pass macros up front. My thoughts on
how to phase this in are that we first use the template/singleton that I had in
my last patches. Then we can update the context to conform to the API, and
migrate the macros and pass construction to pass the context in. After that we
can remove the default constructors from the passes and replace the template
argument with LLVMContext.

Does that sound like a good roadmap for a transition?
> 
> How do you want to go about the review / cleanups on the patch? Maybe a new
thread? I have a couple of significant things and probably a bunch of really
minor cleanup stuff.
Sounds good to me. How about I clean up my patches based on your recommendations
below and start a Phabricator review?

-Chris
> 
> While you may already have much of this fixed, just listing the things
I've seen here so I don't forget:
> 
> - I would use a DenseMap or a sorted vector for the lookups. While not
super hot, std::map is pretty terrible.
> - Naming of parameters likely needs updating to match conventions...
> - I think you can kill off the hidden flag from the new API and just always
mark these as hidden with cl::opt. Any flags which *aren't* marked as hidden
probably shouldn't migrate to this API and instead should become constructor
parameters controlled at API boundaries (and surfacing with non-hidden cl::opt
objects in the tools rather than the libraries).
> - I think you can kill off the builder based on the above. Most of the
builder is already redundant with the register API.
> - Go ahead and clang-format the CommandLine.{h,cpp} code you're going
to need to touch so that the diff is sane.
> 
> I think everything else I see will just fall out of the cleanup.
> 
> Sound like a plan?
> 
> 
> 
> On Fri, Aug 29, 2014 at 4:40 PM, Chris Bieneman <cbieneman at
apple.com> wrote:
> This all sounds fine to me.
> 
> How do you feel about the idea of committing changes with some cleanup from
my last patches and converting a few  cl::opts to the new API to see how the new
API feels? I can provide patches either later today or this weekend.
> 
> -Chris
> 
> On Aug 29, 2014, at 2:14 PM, Chandler Carruth <chandlerc at
google.com> wrote:
> 
>> 
>> On Fri, Aug 29, 2014 at 1:38 PM, Chris Bieneman <cbieneman at
apple.com> wrote:
>> While it is true that we probably don’t need to allow the libraries to
mess around with positional parameters, non-hidden options are (I think) a
different story. If you look at include/llvm/CodeGen/CommandFlags.h, there are a
number of flags defined here that are not positional, not hidden, but only
relevant to the CodeGen library. It is probably reasonable that these flags be
transitioned to the new API, but they also should be exposed on the command
line.
>> 
>> So, the 'hidden' flags are *all* exposed on the commandline,
they just aren't in the *help* output.
>> 
>> My thinking (which perhaps is wrong) is that the library options of
this form probably shouldn't be in the help output. The help output should
probably talk about the tool commandline flags which drive actual API knobs in
constructors of things.
>>  
>> 
>> Aside from those options there are probably very few cases where it
actually makes sense for library options to not be hidden, although I must add
the caveat that I’m sure there are plenty of library options that are not hidden
today and making this change will make them hidden. I’m ok with this, but it
does change one of my original goals because it will actually modify the
behavior of the command line.
>> 
>> This should only change the behavior of '-help' which I think
is fine.
>>> Is that a convincing argument for you to restrict the scope of this
interface? Maybe there are other things that you'd like to do here that
motivate going the other direction?
>> 
>> How do you feel about also providing a fluent-style API for toggling
the (hopefully) uncommon options?
>> 
>> I think that if we have optional things that don't make sense as
one or two unsurprising optional arguments, the fluent API design is the right
one.
>> 
>> But I'd like to understand what those optional properties are.
Currently, all of the ones we've discussed seem fine to either be required,
or be completely omitted.
> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140917/93a6975b/attachment.html>

On Wed, Sep 17, 2014 at 10:35 AM, Chris Bieneman <beanz at apple.com>
wrote:
> On Sep 16, 2014, at 5:28 PM, Chandler Carruth <chandlerc at
google.com>
> wrote:
>
> Picking this back up, I think that your patch with some cleanups is
> probably the right starting point. I think the only really substantive
> issue I currently have is that I'd rather have the OptionRegistry in
the
> LLVMContext rather than in a singleton method. I understand we'll also
have
> to add similar registries to other context objects (MCContext at least),
> but I think it's useful to start off with just one context initially.
>
>
> In my last patches I had the OptionRegistry really just being a template
> type that conformed to the get interface. I was having it be a singleton so
> that we have a way to gradually transition. To make it the context means
> we'll need to change all the pass constructors and the pass macros up
> front. My thoughts on how to phase this in are that we first use the
> template/singleton that I had in my last patches. Then we can update the
> context to conform to the API, and migrate the macros and pass construction
> to pass the context in. After that we can remove the default constructors
> from the passes and replace the template argument with LLVMContext.
>
> Does that sound like a good roadmap for a transition?
>
Hmm, OK. I disagree with some specifics (I don't think we want to do this
in the constructor) but it should be easy to sort that out as it comes up.

>
>
> How do you want to go about the review / cleanups on the patch? Maybe a
> new thread? I have a couple of significant things and probably a bunch of
> really minor cleanup stuff.
>
>
> Sounds good to me. How about I clean up my patches based on your
> recommendations below and start a Phabricator review?

Sure.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140917/a99f27ab/attachment.html>