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

[Pete Cooper]

> 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