[LLVMdev] [RFC] Removing static initializers for command line options
Jim Grosbach
grosbach at apple.com
Fri Aug 29 11:29:49 PDT 2014
> 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>
More information about the llvm-dev
mailing list