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

[Chandler Carruth]

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.

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?

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


I wouldn't set the initial value here -- I think it is more clear and more
flexible to pass that into the "get" API.


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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140829/f5c16d99/attachment.html>