<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Aug 29, 2014, at 1:50 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><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></div></blockquote><div><br></div><div>That makes sense. I’ll think about how to best re-structure the code and propose some patches.</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">
<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></div></blockquote><div><br></div><div>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.</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">
<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></div></blockquote><div><br></div><div>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.</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">
<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></div></blockquote><div><br></div><div>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.</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">
<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></div></blockquote><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><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>
</blockquote><br></div><div>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.</div><div><br></div><div>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?</div><div><br></div><div>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.</div><div><br></div><div>-Chris</div><br></body></html>