<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 27, 2014 at 2:36 PM, Chris Bieneman <span dir="ltr"><<a href="mailto:beanz@apple.com" target="_blank">beanz@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">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.<div>
<br></div><div>Toward that end I’ve put together some patches with a new fluent-style API as a starting point of a conversation.</div><div><br></div><div>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</div>
</div></blockquote><div><br></div><div>Yep, I'll actually only use that part of the patch to comment on, I agree the rest are largely details.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><br></div><div>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:</div>
<div><br></div><div>1) I’ve switched to some crazy template-foo for generating IDs to identify options</div><div>2) I’ve moved all the new API components out of the cl namespace</div><div>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</div>
<div>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</div><div><br></div><div>
As with my previous patches, these patches are designed to work with existing cl::opts.</div></div></blockquote></div><div class="gmail_extra"><br></div><div class="gmail_extra">One very high-level comment about this interaction...</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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).</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">Now, on to the code! =]</div><div class="gmail_extra"><br></div><div class="gmail_extra">diff --git a/lib/Transforms/Scalar/Scalarizer.cpp b/lib/Transforms/Scalar/Scalarizer.cpp</div>
<div class="gmail_extra">index 813041a..d830a48 100644</div><div class="gmail_extra">--- a/lib/Transforms/Scalar/Scalarizer.cpp</div><div class="gmail_extra">+++ b/lib/Transforms/Scalar/Scalarizer.cpp</div><div class="gmail_extra">
@@ -127,9 +127,22 @@ class Scalarizer : public FunctionPass,</div><div class="gmail_extra"> public:</div><div class="gmail_extra"> static char ID;</div><div class="gmail_extra"> </div><div class="gmail_extra">+ template<typename OptStore></div>
<div class="gmail_extra">+ void init(const OptStore &OS) {</div><div class="gmail_extra">+ initializeScalarizerPass(*PassRegistry::getPassRegistry());</div><div class="gmail_extra">+ ScalarizeLoadStore = OS.template</div>
<div class="gmail_extra">+ get<bool, Scalarizer, &Scalarizer::ScalarizeLoadStore>();</div><div class="gmail_extra">+ }</div><div class="gmail_extra">+</div><div class="gmail_extra"> Scalarizer() :</div>
<div class="gmail_extra">
FunctionPass(ID) {</div><div class="gmail_extra">- initializeScalarizerPass(*PassRegistry::getPassRegistry());</div><div class="gmail_extra">+ init(OptionRegistry::instance());</div><div class="gmail_extra">+ }</div>
<div class="gmail_extra">+</div><div class="gmail_extra">+ template<typename OptStore></div><div class="gmail_extra">+ Scalarizer(const OptStore &OS) :</div><div class="gmail_extra">+ FunctionPass(ID) {</div>
<div class="gmail_extra">+ init(OS);</div><div class="gmail_extra"><br></div><div class="gmail_extra">What do you think about my suggestion to use doInitialization(Module &) and getting the options from LLVMContext?</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">@@ -150,6 +163,18 @@ public:<br></div><div class="gmail_extra"> bool visitLoadInst(LoadInst &);</div><div class="gmail_extra">
bool visitStoreInst(StoreInst &);</div><div class="gmail_extra"> </div><div class="gmail_extra">+ static void RegisterOptions() {</div><div class="gmail_extra">+ // This is disabled by default because having separate loads and stores makes</div>
<div class="gmail_extra">+ // it more likely that the -combiner-alias-analysis limits will be reached.</div><div class="gmail_extra">+ OptionRegistry::CreateOption<bool,</div><div class="gmail_extra">+ Scalarizer,</div>
<div class="gmail_extra">+ &Scalarizer::ScalarizeLoadStore>()</div><div class="gmail_extra">+ .setInitialValue(false)</div><div class="gmail_extra">+ .setArgStr("scalarize-load-store")</div>
<div class="gmail_extra">+ .setHiddenFlag(cl::Hidden)</div><div class="gmail_extra">+ .setDescription("Allow the scalarizer pass to scalarize loads and store");</div><div class="gmail_extra"><br></div>
<div class="gmail_extra">As this is a new interface, I would follow the new naming conventions here.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Also, I don't think it being a static method is really useful. What about just making this a free function "registerOption"?</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">I wouldn't set the initial value here -- I think it is more clear and more flexible to pass that into the "get" API.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">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:</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra"> static void registerOptions() {</div><div class="gmail_extra"> registerOption<bool, Scalarizer, &Scalarizer::ScalarizeLoadStore>(</div>
<div class="gmail_extra"> "scalarize-load-store",</div><div class="gmail_extra"> OptionOptions</div><div class="gmail_extra"> .hidden()</div><div class="gmail_extra"> .description("Allow the scalarizer pass to scalarize loads and stores"));</div>
<div class="gmail_extra"> }</div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">2) The name of the option is pretty clearly something that should be required I think.</div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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?</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">At that point, I think this becomes something pleasingly simple:</div><div class="gmail_extra"><br>
</div><div class="gmail_extra"><div class="gmail_extra"> static void registerOptions() {</div><div class="gmail_extra"> registerOption<bool, Scalarizer, &Scalarizer::ScalarizeLoadStore>(</div><div class="gmail_extra">
"scalarize-load-store",</div><div class="gmail_extra"> "Allow the scalarizer pass to scalarize loads and stores");<br></div><div class="gmail_extra"> }</div><div><br></div></div><div class="gmail_extra">
Thoughts?</div></div></div>