[llvm-dev] [RFC] Enable thread specific cl::opt values for multi-threaded support

Tue Oct 30 22:47:09 PDT 2018

Here are copies of some key comments from the patch
    https://reviews.llvm.org/D53424 Enable thread specific cl::opt values
for multi-threaded support
So, please continue discussion here.

>>! In D53424#1272419, @delcypher wrote:
> @yrouban While I agree with what you're trying to achieve, I'm not a huge
fan of the implementation.
>
> * LLVM’s global options are great for easily changing things during
development but they are terrible from a library user perspective. If the
default values of these options are not good for a user it really suggests
they need to be moved into a proper API.
> * Given that `cl::opt`’s are usually global it feels extremely weird that
reading and writing to these could actually be a thread local operation.
>
> On the other hand what you've done seems like it could be reasonable
stop-gap to allow you to make progress. However I really don't want to see
what's in this patch become permanent unless there is a strong commitment
from the community to start cleaning up our global options mess so that
this patch can eventually be removed.

==============================
>>! In D53424#1273950, @delcypher wrote:
>>>! In D53424#1273737, @yrouban wrote:
>>>>! In D53424#1273728, @jfb wrote:
>>> Was there a commitment from the community, with some time horizon for
moving away from this patch?
>> Frankly speaking, I do not think that this kind of transition can be
done easily. Every single option needs a big change. That is the biggest
disadvantage of the feature D5389. It needs some improvements to allow
enough flexibility. And we cannot know what improvements are needed until
we implement enough options in the new way. I do not see any good way to
force the transition to finish in a fixed time frame. If D5389 were easy to
use and transit to, it would have been already adopted and we would not
have to came up with another idea like this D53424.
>> In other words, I would give the new idea a try (at least under a
preprocessor flag).
>
> While I agree that a transition away from global `cl::opt`s is going to
be tough I think we need to get agreement from the community on some sort
of plan before we land this. Otherwise we'll end up with this patch
becoming a permanent part of LLVM's internal API which makes our technical
debt even worse.
> My suggestion would be to create a new RFC on llvm-dev that proposes a
plan to move away from `cl::opt`s in LLVM's codebase (and probably other
sub-projects). Given that you're looking at this issue you're probably in
good position to discuss the technical problems with the available
replacement APIs in LLVM's codebase.

==============================

>>! In D53424#1273975, @nhaehnle wrote:
> May I ask why you think it's important to move away from `cl::opt` in the
first place? What purpose does it actually solve?
>
> I think having global `cl::opt` variables as descriptions of available
options is a perfectly pragmatic thing to do. The storage of the chosen
option values can be decoupled of the description of the options, which is
something that this patch essentially does.
>
> I mean, I get the idea that there are //certain kinds// of options which
are associated to passes that you may run multiple times in a compilation
flow, and you may want to be able to specify different option values for
the different invocations of those passes; and yeah, those options are not
served well by `cl::opt`. But many, perhaps most, options are not like
this. Having them described as global variables is perfectly fine as far as
I can tell.
>
> Conversely, forcing such kinds of options into a new option struct has a
clear disadvantage, which is that adding a new option (or possibly even
modifying an existing one) will change a header file that will end up being
included by essentially the whole world, so that adding a single option
will force a recompile of large parts of the project, which is always a
pain.
>
> So from a pragmatic point of view, I think there is actually a strong
reason for keeping `cl::opt`s around //as a means of describing available
options//, even while decoupling storage for option values. I'm sure
there's a fancy name to describe this kind of pattern, if that would help
convince you of it. It's a really bad idea to start churn all over the code
base unless you can actually name a clear benefit for that churn. The two
items you've brought up so far are:
>
>> * LLVM’s global options are great for easily changing things during
development but they are terrible from a library user perspective. If the
default values of these options are not good for a user it really suggests
they need to be moved into a proper API.
>
> I agree that LLVMParseCommandLine is not a great interface, but there's
really nothing stopping us from adding a function
`LLVMSetCommandLineOptionBool/Int/String(option_name, value)`. That's a
perfectly fine API.
>
>> * Given that `cl::opt`’s are usually global it feels extremely weird
that reading and writing to these could actually be a thread local
operation.
>
> That's purely aesthetic opinion and not an argument at all.

==============================

>>! In D53424#1278545, @delcypher wrote:
>>>! In D53424#1273975, @nhaehnle wrote:
>> May I ask why you think it's important to move away from `cl::opt` in
the first place? What purpose does it actually solve?
>
> I'm not saying that we should move away from `cl::opt` everywhere. My
opinion is that these really should only be used for developer only
debugging. For other options that we wish to expose to the user, these
really should be part of a proper API.
> `cl::opt` are typically declared in source files with no corresponding
declaration in a header file which means getting access to them to
read/write to them is pain because you have to write your own declaration
to get at them. Aside from this API annoyance `cl::opt`s are usually global
which means that
>
> * They are not safe to use in a multi-threaded environment if one of the
threads wishes to write to the `cl::opt`.
> * A whole bunch of global constructors  (one for every `cl::opt`
definition) run at library load time. Clients of LLVM might not actually
want this.
>
>>> * Given that `cl::opt`’s are usually global it feels extremely weird
that reading and writing to these could actually be a thread local
operation.
>>
>> That's purely aesthetic opinion and not an argument at all.
>
> It absolutely is an argument. If the patch changes the behaviour in a
non-intuitive way then we need to consider this and the impact it will have
on its users. Globals that don't act like globals is not I what I would
call intuitive.
> Pretending like this doesn't matter seems overly dismissive.
>
> Anyway this discussion should move to the RFC.

==============================

On Fri, Oct 19, 2018 at 5:58 PM Yevgeny Rouban <yrouban at gmail.com> wrote:

> Hello LLVM Developers.
>
>
>
> We at Azul Systems are working on a multi-threaded LLVM based compiler. It
> can run several compilations each of which compiles its own module in its
> own thread.
>
> One of the limitation we face is that all threads use the same options
> (instances of cl::opt). In other words, the options are global and cannot
> be changed for one thread and left unchanged for the others.
>
>
>
> One solution I propose in the patch
>
> https://reviews.llvm.org/D53424 Enable thread specific cl::opt values for
> multi-threaded support
>
>
>
> As the change affects many source files (though slightly) I decided to
> share it with wider audience. Any less intrusive solution is welcome.
>
>
>
> Here is the patch description for your convenience:
>
> ===
>
> When several threads compile different modules the compiler options
> (instances of cl::opt) cannot be set individually for each thread. That is
> because the options are visible to all threads. In other words all options
> are global.
>
>
>
> It would be convenient if the options were specific to LLVMContext and
> they were accessed through an instance of LLVMContext. This kind of change
> would need changes in all source files where options are used.
>
>
>
> This patch proposes a solution that needs minimal changes in LLVM source
> base.
>
> It is proposed to have a thread local set of re-defined option values
> mapped by pointers to options.
>
> Specifically, every time a program gets/sets a value for an option it is
> checked if the current thread local context is set for the current thread
> and the option has its local copy in this context. If so the local copy of
> the option is accessed, otherwise the global option is accessed. For all
> programs that existed so far the context is not set and they work with the
> global options. For new multi-threaded compilers (where every thread
> compiles its own module) every thread can be linked to its own context (see
> ContextValues) where any option can have its thread specific value that do
> not affect the other threads' option values. See the thread_routine() in
> the test ContextSpecificValues2.
>
>
>
> This feature allows a configuration flexibility for multi-threaded
> compilers that can compile every compilation unit in its own thread with
> different command line options.
>
> ===
>
>
>
> Thanks.
>
> -Yevgeny Rouban
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20181031/cb548c9e/attachment-0001.html>