Defining a new API for debug options that doesn't rely on static global cl::opts.

[beanz]

Summary

This is based on the discussions from the LLVMDev thread:
http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-August/075886.html

[chandlerc]

I think the API used by the pass is really good here. Maybe some *minor* tweaks, but generally, this seems fine. Totally fine to commit.

Most of my comments below are on the implementation of things. The big thing I think should get addressed before this goes in are docs for the option API so folks know how to use it, and sinking the cl::Option stuff into Options.cpp (if that proves possible).

Thanks, and sorry for the delays here.

include/llvm/Support/CommandLine.h

35–48 ↗

(On Diff #13814)

Most of this is unused now?

I would reverse the dependency relationship here.

I would have Option.h provide the key type as a member type of the registry, and include it here. Then I would sink tall of the cl::Option building stuff into the implementation file. If this proves hard, feel free to leave the OptionKey business here, but nuke the rest.

182 ↗

(On Diff #13814)

If you end up needing this, just remove the 'llvm::' and you won't need to forward declare the class.

include/llvm/Support/Options.h

1 ↗

(On Diff #13814)

Need normal file template...

7 ↗

(On Diff #13814)

Doesn't seem like a useful comment...

14–15 ↗

(On Diff #13814)

What are the ownership semantics here? i feel like these really need better comments.

17 ↗

(On Diff #13814)

Not a useful comment.

29 ↗

(On Diff #13814)

Usually "I" or "It" but not "IT".

lib/Support/Options.cpp

17–21 ↗

(On Diff #13814)

Oof, this is the only thing requiring the cl::Option to be aware of the key? Do we really need this API?

Considering that the registration is expected to happen during "initialization" my inclination is to make it monotonic and remove the ability to unregister options. Does that make sense to you?

lib/Transforms/Scalar/Scalarizer.cpp

154–156 ↗

(On Diff #13814)

Should re-wrap comment.

[beanz]

Thanks for the feedback. Sorry it has taken me a while to reply. The suggestions all look good. I'm working on revising the patches, and will upload an update shortly.

More comments below.

include/llvm/Support/CommandLine.h

35–48 ↗

(On Diff #13814)

The opt_builder was definitely just something I missed cleaning up my earlier patches, and if I rip out the removeOption code as you suggest I can move the LLVMOptionKey to Options.h and get rid of the rest of this.

Ultimately I'd like to invert the relationship as you mention and have all of cl::Option be in Options.h, but I'd like to leave that for later when we can re-design how the innards of all the cl::opt stuff works. Does that sound reasonable to you?

lib/Support/Options.cpp

17–21 ↗

(On Diff #13814)

I think it is reasonable to remove the ability to remove options.

[beanz]

These patches include updates based on Chandler's feedback:

• Now there are no changes to CommandLine.h
• Options.h and Options.cpp have minor code cleanup and style adjustments
• OptionRegistry no longer supports removing options. This support wasn't really used anyways.
• Formatting fixup in Scalarizer.cpp

[beanz]

I noticed a few comments that were incorrect in the new file headers. These patches update those headers.

[chandlerc]

Again, the API in the pass seems pretty good. More comments on the implementation.

include/llvm/IR/LLVMContext.h

167–170 ↗

(On Diff #14345)

This needs some documentation. =]

include/llvm/PassSupport.h

87 ↗

(On Diff #14345)

We should use the new naming conventions here so we aren't establishing new non-conventional API requirements.

include/llvm/Support/Options.h

21–22 ↗

(On Diff #14345)

This comment doesn't really make sense to me. Not sure what its trying to say...

Maybe just: 'Options are keyed off the unique address of a static character, potentially one synthesized based on a particular set of template arguments'?

23 ↗

(On Diff #14345)

Not sure giving this type a name really helps.

I would just use a 'void *' as the key type internally, and the class template as the external interface? If you want to make the 'void *' raw key type available for clients, I would typedef 'OptionKey' to 'void *', and name the template for synthesizing a static variable whose address is a suitable 'void *' key for a set of template arguments something like 'OptionKeyFromTemplateArgs'.

31 ↗

(On Diff #14345)

Why initialize it at all?

33–35 ↗

(On Diff #14345)

Please use modern doxygen syntax here and elsewhere. Also it would be good to give the class a reasonable introductory comment.

38 ↗

(On Diff #14345)

Should document that this owns the cl::Option objects pointed to but doesn't use std::unique_ptr or delete them but instead intentionally leaks them.

45–46 ↗

(On Diff #14345)

Shouldn't these be private? Only a single global instance of this registry can exist in order to make the ownership semantics of the cl::Option objects reasonable?

[beanz]

One comment below. I will update the patches to reflect the other feedback.

include/llvm/Support/Options.h

45–46 ↗

(On Diff #14345)

These can't be private unless I declare ManagedStatic as a friend, because they are invoked from ManagedStatic, which I think somewhat defeats the point.

[beanz]

A few more comments for things I came across while updating the patches. Updated patches coming shortly.

include/llvm/Support/Options.h

31 ↗

(On Diff #14345)

If you don't initialize this you get a warning "has internal linkage but is not defined [-Wundefined-internal]"

38 ↗

(On Diff #14345)

But it does delete them... See the destructor in Options.cpp.

[beanz]

I've removed the typedef as per chandlerc's comments, and I've updated the formatting and added comments throughout.

[chandlerc]

Mostly just getting the comments cleared up with good documentation for this new API.

include/llvm/IR/LLVMContext.h

167–168 ↗

(On Diff #14750)

Again, follow the modern guidelines for doxygen.

include/llvm/Support/Options.h

10–12 ↗

(On Diff #14750)

I would turn this into a doxygen comment using the '\file' directive. See iterator_range.h for an example.

I would also specifically add an example of how to use this API to add an option to the pass.

21–29 ↗

(On Diff #14750)

I would put all of this into a 'detail' namespace. Users of the API shouldn't really be aware of it.

31 ↗

(On Diff #14750)

A brief summary here would really help. Also, things like "This class" don't really contribute much. For example:

/// \brief Singleton class used to register debugging options.
///
/// ...

39–40 ↗

(On Diff #14750)

Modern doxygen.

47 ↗

(On Diff #14750)

Modern doxygen. I'm going to stop making this comment, but *please* actually check all your API documentation.

[beanz]

This update contains changes based on chandlerc's feedback.

Changes in these patches:

• Moved the OptionKey into a detail namespace
• Updated comments based on chandlerc's feedback

Since the API seems stabilized and the feedback at this point is mostly documentation can we transition this to post-commit review for further feedback?

[chandlerc]

Sorry for the delay, but I do think it is really important to get basic API documentation right.

Anyways, this looks much better as an initial iteration on the API. Thanks, and please commit.

include/llvm/Support/Options.h

9 ↗

(On Diff #14963)

its '\file'

[beanz]

Thanks! I agree getting the docs right is important, and I'm really happy with where this API has ended up.

Closed by commit rL219854 (authored by cbieneman).

llvm/trunk/include/llvm/IR/LLVMContext.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_IR_LLVMCONTEXT_H
 #define LLVM_IR_LLVMCONTEXT_H
 
 #include "llvm-c/Core.h"
 #include "llvm/Support/CBindingWrapping.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/Options.h"
 
 namespace llvm {
 
 class LLVMContextImpl;
 class StringRef;
 class Twine;
 class Instruction;
 class Module;
public:
   /// with optional location information.  This function returns, so code should
   /// be prepared to drop the erroneous construct on the floor and "not crash".
   /// The generated code need not be correct.  The error message will be
   /// implicitly prefixed with "error: " and should not end with a ".".
   void emitError(unsigned LocCookie, const Twine &ErrorStr);
   void emitError(const Instruction *I, const Twine &ErrorStr);
   void emitError(const Twine &ErrorStr);
 
+  /// \brief Query for a debug option's value.
+  ///
+  /// This function returns typed data populated from command line parsing.
+  template <typename ValT, typename Base, ValT(Base::*Mem)>
+  ValT getOption() const {
+    return OptionRegistry::instance().template get<ValT, Base, Mem>();
+  }
+
 private:
   LLVMContext(LLVMContext&) LLVM_DELETED_FUNCTION;
   void operator=(LLVMContext&) LLVM_DELETED_FUNCTION;
 
   /// addModule - Register a module as being instantiated in this context.  If
   /// the context is deleted, the module will be deleted as well.
   void addModule(Module*);
 

llvm/trunk/include/llvm/PassSupport.h

PassInfo *PI = new PassInfo(name, arg, & passName ::ID, \
       PassInfo::NormalCtor_t(callDefaultCtor< passName >), cfg, analysis); \
     Registry.registerPass(*PI, true); \
     return PI; \
   } \
   void llvm::initialize##passName##Pass(PassRegistry &Registry) { \
     CALL_ONCE_INITIALIZATION(initialize##passName##PassOnce) \
   }
 
+#define INITIALIZE_PASS_WITH_OPTIONS(PassName, Arg, Name, Cfg, Analysis) \
+  INITIALIZE_PASS_BEGIN(PassName, Arg, Name, Cfg, Analysis) \
+  PassName::registerOptions(); \
+  INITIALIZE_PASS_END(PassName, Arg, Name, Cfg, Analysis)
+
+#define INITIALIZE_PASS_WITH_OPTIONS_BEGIN(PassName, Arg, Name, Cfg, Analysis) \
+  INITIALIZE_PASS_BEGIN(PassName, Arg, Name, Cfg, Analysis) \
+  PassName::registerOptions(); \
+
 template<typename PassName>
 Pass *callDefaultCtor() { return new PassName(); }
 
 template <typename PassName> Pass *callTargetMachineCtor(TargetMachine *TM) {
   return new PassName(TM);
 }
 
 //===---------------------------------------------------------------------------

llvm/trunk/include/llvm/Support/Options.h

+//===- llvm/Support/Options.h - Debug options support -----------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+/// \file
+/// This file declares helper objects for defining debug options that can be
+/// configured via the command line. The new API currently builds on the cl::opt
+/// API, but does not require the use of static globals.
+///
+/// With this API options are registered during initialization. For passes, this
+/// happens during pass initialization. Passes with options will call a static
+/// registerOptions method during initialization that registers options with the
+/// OptionRegistry. An example implementation of registerOptions is:
+///
+/// static void registerOptions() {
+///   OptionRegistry::registerOption<bool, Scalarizer,
+///                                &Scalarizer::ScalarizeLoadStore>(
+///       "scalarize-load-store",
+///       "Allow the scalarizer pass to scalarize loads and store", false);
+/// }
+///
+/// When reading data for options the interface is via the LLVMContext. Option
+/// data for passes should be read from the context during doInitialization. An
+/// example of reading the above option would be:
+///
+/// ScalarizeLoadStore =
+///   M.getContext().template getOption<bool,
+///                                     Scalarizer,
+///                                     &Scalarizer::ScalarizeLoadStore>();
+///
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/CommandLine.h"
+
+namespace llvm {
+
+namespace detail {
+
+// Options are keyed of the unique address of a static character synthesized
+// based on template arguments.
+template <typename ValT, typename Base, ValT(Base::*Mem)> class OptionKey {
+public:
+  static char ID;
+};
+
+template <typename ValT, typename Base, ValT(Base::*Mem)>
+char OptionKey<ValT, Base, Mem>::ID = 0;
+
+} // namespace detail
+
+/// \brief Singleton class used to register debug options.
+///
+/// The OptionRegistry is responsible for managing lifetimes of the options and
+/// provides interfaces for option registration and reading values from options.
+/// This object is a singleton, only one instance should ever exist so that all
+/// options are registered in teh same place.
+class OptionRegistry {
+private:
+  DenseMap<void *, cl::Option *> Options;
+
+  /// \brief Adds a cl::Option to the registry.
+  ///
+  /// \param Key unique key for option
+  /// \param O option to map to \p Key
+  ///
+  /// Allocated cl::Options are owened by the OptionRegistry and are deallocated
+  /// on destruction or removal
+  void addOption(void *Key, cl::Option *O);
+
+public:
+  ~OptionRegistry();
+  OptionRegistry() {}
+
+  /// \brief Returns a reference to the singleton instance.
+  static OptionRegistry &instance();
+
+  /// \brief Registers an option with the OptionRegistry singleton.
+  ///
+  /// \param ValT type of the option's data
+  /// \param Base class used to key the option
+  /// \param Mem member of \p Base used for keying the option
+  ///
+  /// Options are keyed off the template parameters to generate unique static
+  /// characters. The template parameters are (1) the type of the data the
+  /// option stores (\p ValT), the class that will read the option (\p Base),
+  /// and the memeber that the class will store the data into (\p Mem).
+  template <typename ValT, typename Base, ValT(Base::*Mem)>
+  static void registerOption(const char *ArgStr, const char *Desc,
+                             const ValT &InitValue) {
+    cl::opt<ValT> *Option = new cl::opt<ValT>(ArgStr, cl::desc(Desc),
+                                              cl::Hidden, cl::init(InitValue));
+    instance().addOption(&detail::OptionKey<ValT, Base, Mem>::ID, Option);
+  }
+
+  /// \brief Returns the value of the option.
+  ///
+  /// \param ValT type of the option's data
+  /// \param Base class used to key the option
+  /// \param Mem member of \p Base used for keying the option
+  ///
+  /// Reads option values based on the key generated by the template parameters.
+  /// Keying for get() is the same as keying for registerOption.
+  template <typename ValT, typename Base, ValT(Base::*Mem)> ValT get() const {
+    auto It = Options.find(&detail::OptionKey<ValT, Base, Mem>::ID);
+    assert(It != Options.end() && "Option not in OptionRegistry");
+    return *(cl::opt<ValT> *)It->second;
+  }
+};
+
+} // namespace llvm

llvm/trunk/lib/Support/CMakeLists.txt

add_llvm_library(LLVMSupport
   LineIterator.cpp
   Locale.cpp
   LockFileManager.cpp
   ManagedStatic.cpp
   MathExtras.cpp
   MemoryBuffer.cpp
   MemoryObject.cpp
   MD5.cpp
+  Options.cpp
   PluginLoader.cpp
   PrettyStackTrace.cpp
   RandomNumberGenerator.cpp
   Regex.cpp
   ScaledNumber.cpp
   SmallPtrSet.cpp
   SmallVector.cpp
   SourceMgr.cpp

llvm/trunk/lib/Support/CommandLine.cpp

void Option::addArgument() {
   assert(!NextRegistered && "argument multiply registered!");
 
   NextRegistered = RegisteredOptionList;
   RegisteredOptionList = this;
   MarkOptionsChanged();
 }
 
 void Option::removeArgument() {
-  assert(NextRegistered && "argument never registered");
-  assert(RegisteredOptionList == this && "argument is not the last registered");
+  if (RegisteredOptionList == this) {
     RegisteredOptionList = NextRegistered;
     MarkOptionsChanged();
+    return;
+  }
+  Option *O = RegisteredOptionList;
+  for (; O->NextRegistered != this; O = O->NextRegistered)
+    ;
+  O->NextRegistered = NextRegistered;
+  MarkOptionsChanged();
 }
 
 // This collects the different option categories that have been registered.
 typedef SmallPtrSet<OptionCategory*,16> OptionCatSet;
 static ManagedStatic<OptionCatSet> RegisteredOptionCategories;
 
 // Initialise the general option category.
 OptionCategory llvm::cl::GeneralCategory("General options");

llvm/trunk/lib/Support/Options.cpp

+//===- llvm/Support/Options.cpp - Debug options support ---------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements the helper objects for defining debug options using the
+// new API built on cl::opt, but not requiring the use of static globals.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/Options.h"
+#include "llvm/Support/ManagedStatic.h"
+
+using namespace llvm;
+
+OptionRegistry::~OptionRegistry() {
+  for (auto IT = Options.begin(); IT != Options.end(); ++IT)
+    delete IT->second;
+}
+
+void OptionRegistry::addOption(void *Key, cl::Option *O) {
+  assert(Options.find(Key) == Options.end() &&
+         "Argument with this key already registerd");
+  Options.insert(std::make_pair(Key, O));
+}
+
+static ManagedStatic<OptionRegistry> OR;
+
+OptionRegistry &OptionRegistry::instance() { return *OR; }

llvm/trunk/lib/Transforms/Scalar/Scalarizer.cpp

public:
   bool visitGetElementPtrInst(GetElementPtrInst &);
   bool visitCastInst(CastInst &);
   bool visitBitCastInst(BitCastInst &);
   bool visitShuffleVectorInst(ShuffleVectorInst &);
   bool visitPHINode(PHINode &);
   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::registerOption<bool, Scalarizer,
+                                 &Scalarizer::ScalarizeLoadStore>(
+        "scalarize-load-store",
+        "Allow the scalarizer pass to scalarize loads and store", false);
+  }
+
 private:
   Scatterer scatter(Instruction *, Value *);
   void gather(Instruction *, const ValueVector &);
   bool canTransferMetadata(unsigned Kind);
   void transferMetadata(Instruction *, const ValueVector &);
   bool getVectorLayout(Type *, unsigned, VectorLayout &);
   bool finish();
 
   template<typename T> bool splitBinary(Instruction &, const T &);
 
   ScatterMap Scattered;
   GatherList Gathered;
   unsigned ParallelLoopAccessMDKind;
   const DataLayout *DL;
+  bool ScalarizeLoadStore;
 };
 
 char Scalarizer::ID = 0;
 } // end anonymous namespace
 
-// This is disabled by default because having separate loads and stores makes
-// it more likely that the -combiner-alias-analysis limits will be reached.
-static cl::opt<bool> ScalarizeLoadStore
-  ("scalarize-load-store", cl::Hidden, cl::init(false),
-   cl::desc("Allow the scalarizer pass to scalarize loads and store"));
-
-INITIALIZE_PASS(Scalarizer, "scalarizer", "Scalarize vector operations",
-                false, false)
+INITIALIZE_PASS_WITH_OPTIONS(Scalarizer, "scalarizer",
+                             "Scalarize vector operations", false, false);
 
 Scatterer::Scatterer(BasicBlock *bb, BasicBlock::iterator bbi, Value *v,
                      ValueVector *cachePtr)
   : BB(bb), BBI(bbi), V(v), CachePtr(cachePtr) {
   Type *Ty = V->getType();
   PtrTy = dyn_cast<PointerType>(Ty);
   if (PtrTy)
     Ty = PtrTy->getElementType();
if (PtrTy) {
     CV[I] = Builder.CreateExtractElement(V, Builder.getInt32(I),
                                          V->getName() + ".i" + Twine(I));
   }
   return CV[I];
 }
 
 bool Scalarizer::doInitialization(Module &M) {
   ParallelLoopAccessMDKind =
       M.getContext().getMDKindID("llvm.mem.parallel_loop_access");
+  ScalarizeLoadStore =
+      M.getContext()
+          .template getOption<bool, Scalarizer, &Scalarizer::ScalarizeLoadStore>();
   return false;
 }
 
 bool Scalarizer::runOnFunction(Function &F) {
   DataLayoutPass *DLP = getAnalysisIfAvailable<DataLayoutPass>();
   DL = DLP ? &DLP->getDataLayout() : nullptr;
   for (Function::iterator BBI = F.begin(), BBE = F.end(); BBI != BBE; ++BBI) {
     BasicBlock *BB = BBI;