<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>Here are
copies
of some key comments from the patch</div><div> <a href="https://reviews.llvm.org/D53424">https://reviews.llvm.org/D53424</a> Enable thread specific cl::opt values for multi-threaded support</div><div>So, please continue discussion here.<br></div><div dir="ltr"><br>>>! In D53424#1272419, @delcypher wrote:<br>> @yrouban While I agree with what you're trying to achieve, I'm not a huge fan of the implementation. <br>> <br>> * 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.<br>> * 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.<br>> <br>> 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.<br></div><div dir="ltr"><br></div><div dir="ltr">==============================</div><div dir="ltr">>>! In D53424#1273950, @delcypher wrote:<br>>>>! In D53424#1273737, @yrouban wrote:<br>>>>>! In D53424#1273728, @jfb wrote:<br>>>> Was there a commitment from the community, with some time horizon for moving away from this patch?<br>>> 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.<br>>> In other words, I would give the new idea a try (at least under a preprocessor flag).<br>> <br>> 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.<br>> 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.</div><div dir="ltr"><br></div><div dir="ltr">
<div dir="ltr">==============================</div>
</div><div dir="ltr"><br></div><div dir="ltr">>>! In D53424#1273975, @nhaehnle wrote:<br>> 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?<br>> <br>> 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.<br>> <br>> 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.<br>> <br>> 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.<br>> <br>> 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:<br>> <br>>> * 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.<br>> <br>> 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.<br>> <br>>> * 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.<br>> <br>> That's purely aesthetic opinion and not an argument at all.<br><br>
<div dir="ltr">
<div dir="ltr">==============================</div>
</div><div dir="ltr"><br></div>
>>! In D53424#1278545, @delcypher wrote:<br>>>>! In D53424#1273975, @nhaehnle wrote:<br>>> 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?<br>> <br>> 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.<br>> `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<br>> <br>> * They are not safe to use in a multi-threaded environment if one of the threads wishes to write to the `cl::opt`.<br>> * 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.<br>> <br>>>> * 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.<br>>> <br>>> That's purely aesthetic opinion and not an argument at all.<br>> <br>> 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.<br>> Pretending like this doesn't matter seems overly dismissive.<br>> <br>> Anyway this discussion should move to the RFC.<br><br>
<div dir="ltr">
<div dir="ltr">==============================</div>
</div><div dir="ltr"><br></div>
</div></div></div></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Oct 19, 2018 at 5:58 PM Yevgeny Rouban <<a href="mailto:yrouban@gmail.com">yrouban@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>Hello LLVM Developers.<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span><span> </span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>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.<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>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.<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span><span> </span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>One solution I propose in
the patch<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span><a href="https://reviews.llvm.org/D53424" style="color:rgb(5,99,193);text-decoration:underline" target="_blank">https://reviews.llvm.org/D53424</a>
Enable thread specific cl::opt values for multi-threaded support<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span><span> </span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>As the change affects many
source files (though slightly) I decided to share it with wider audience. Any
less intrusive solution is welcome.<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span><span> </span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>Here is the patch
description for your convenience:<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>===<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>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.<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span><span> </span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>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.<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span><span> </span></span></p>
<p class="MsoNormal" style="margin:0in 0in 12pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>This patch proposes a solution that needs minimal changes in LLVM source
base.<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>It is proposed to have a
thread local set of re-defined option values mapped by pointers to options.<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>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.<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span><span> </span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>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.<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span>===<span></span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span><span> </span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif">Thanks.<span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif">-Yevgeny Rouban<span></span></p>
<p class="MsoNormal" style="margin:0in 0in 0.0001pt;font-size:11pt;font-family:"Calibri",sans-serif"><span> </span></p>
</div>
</blockquote></div>