[LLVMdev] [RFC] Removing static initializers for command line options

[Pete Cooper]

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