Delay initialization of OptBisect

[kparzysz]

Summary

When LLVM is used in other projects, it may happen that global constructors will execute before the call to ParseCommandLineOptions. [Thanks to @dblaikie for the correction.]

In a project I'm working with there are multiple LLVM codegen executions (targeting different architectures). The OptBisect object is initialized (via a constructor) the first time it's queried by skipFunction, and has no ability to be updated at a later time.

When debugging codegen for the second target, passing -opt-bisect-limit to ParseCommandLineOptions will no longer have any effect if it takes place after the OptBisect object has already been created.

To avoid this problem use a cl::cb (callback) to set the bisection limit when the option is actually processed.

[kparzysz]

Committed by accident.

[dblaikie]

What situation are you dealing with where OptBisect is created during global constructors? OptBisect uses ManagedStatic which only creates the object on first access, right? So maybe it'd be suitable to change the code so it doesn't try to access OptBisect during global construction?

[kparzysz]

Sorry, I was incorrect in my original statement. I have fixed it.

[dblaikie]

In D104551#2833161, @kparzysz wrote:

Sorry, I was incorrect in my original statement. I have fixed it.

Ah, thanks - maybe that's a motivation/opportunity to revisit the design here and make it more scalable. Moving this state into the LLVMContext, for instance, if possible? (I guess most of these choices were considered in the past - but with new use cases it might motivate re-examining & trying a bit more to find ways to avoid this global mutable state)

[kparzysz]

I'm open to that, but it would likely take a while to come up with, and implement an alternative.

Would you be opposed to merging this as a workaround for the time being?

[dblaikie]

In D104551#2833438, @kparzysz wrote:

I'm open to that, but it would likely take a while to come up with, and implement an alternative.

Would you be opposed to merging this as a workaround for the time being?

Oh, I'm only offering suggestions, not mandates/requirements - I'll leave it up to folks more familiar with OptBisect to offer approvals/disapprovals, etc.

[kparzysz]

Ping.

[kparzysz]

Ping.

[andrew.w.kaylor]

Sorry for the non-response, I hadn't notice this review earlier.

Can you describe the flow of execution that causes the option to be parsed after the bisector is initialized? Do you parse the options, run passes, and then parse a different set of options for the second target within the same compilation process? Does your patch need to reset CurBisectNum?

[kparzysz]

Yes, that's exactly what happens. The project uses LLVM as a code generator for multiple targets. The OptBisector object is constructed the first time it's queried by shouldRunPass, which happened during the code generation for the first target. We set up the second target by passing some extra flags to ParseCommandLineOptions, but passing -opt-bisect-limit at this time no longer has any effect (i.e. the setting from the construction time remains).

[andrew.w.kaylor]

In D104551#2909024, @kparzysz wrote:

Yes, that's exactly what happens. The project uses LLVM as a code generator for multiple targets. The OptBisector object is constructed the first time it's queried by shouldRunPass, which happened during the code generation for the first target. We set up the second target by passing some extra flags to ParseCommandLineOptions, but passing -opt-bisect-limit at this time no longer has any effect (i.e. the setting from the construction time remains).

That case is more complex than opt-bisect was originally designed for, but I guess if you have a way to independently specify the bisect limit for each target, you should be able to make it work. To make the implementation more general, I'd suggest that the CurBisectNum should be reset between the targets and probably any time the limit is changed.

Replacing the default OptBisect with a custom OptGate might be a better solution. That said, I wouldn't be opposed to your current change as a quick and possibly temporary solution if you add something to reset the counter.

[kparzysz]

Added resetting of the counter when new limit is set.

[kparzysz]

Added definition of OptBisect::Disabled.

[andrew.w.kaylor]

lgtm

llvm/include/llvm/IR/OptBisect.h

 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_IR_OPTBISECT_H
 #define LLVM_IR_OPTBISECT_H
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ManagedStatic.h"
+#include <limits>
 
 namespace llvm {
 
 class Pass;
 
 /// Extensions to this class implement mechanisms to disable passes and
 /// individual optimizations at compile time.
 class OptPassGate {

 };
 
 /// This class implements a mechanism to disable passes and individual
 /// optimizations at compile time based on a command line option
 /// (-opt-bisect-limit) in order to perform a bisecting search for
 /// optimization-related problems.
 class OptBisect : public OptPassGate {
 public:
-  /// Default constructor, initializes the OptBisect state based on the
-  /// -opt-bisect-limit command line argument.
-  ///
-  /// By default, bisection is disabled.
-  ///
+  /// Default constructor. Initializes the state to "disabled". The bisection
+  /// will be enabled by the cl::opt call-back when the command line option
+  /// is processed.
   /// Clients should not instantiate this class directly.  All access should go
   /// through LLVMContext.
-  OptBisect();
+  OptBisect() = default;
 
   virtual ~OptBisect() = default;
 
   /// Checks the bisect limit to determine if the specified pass should run.
   ///
   /// This forwards to checkPass().
   bool shouldRunPass(const Pass *P, StringRef IRDescription) override;
 
   /// isEnabled() should return true before calling shouldRunPass().
-  bool isEnabled() const override { return BisectEnabled; }
+  bool isEnabled() const override { return BisectLimit != Disabled; }
+
+  /// Set the new optimization limit and reset the counter. Passing
+  /// OptBisect::Disabled disables the limiting.
+  void setLimit(int Limit) {
+    BisectLimit = Limit;
+    LastBisectNum = 0;
+  }
 
   /// Checks the bisect limit to determine if the specified pass should run.
   ///
   /// If the bisect limit is set to -1, the function prints a message describing
   /// the pass and the bisect number assigned to it and return true.  Otherwise,
   /// the function prints a message with the bisect number assigned to the
   /// pass and indicating whether or not the pass will be run and return true if
   /// the bisect limit has not yet been exceeded or false if it has.
   ///
   /// Most passes should not call this routine directly. Instead, they are
   /// called through helper routines provided by the pass base classes.  For
   /// instance, function passes should call FunctionPass::skipFunction().
   bool checkPass(const StringRef PassName, const StringRef TargetDesc);
 
+  static const int Disabled = std::numeric_limits<int>::max();
+
 private:
-  bool BisectEnabled = false;
-  unsigned LastBisectNum = 0;
+  int BisectLimit = Disabled;
+  int LastBisectNum = 0;
 };
 
 /// Singleton instance of the OptBisect class, so multiple pass managers don't
 /// need to coordinate their uses of OptBisect.
 extern ManagedStatic<OptBisect> OptBisector;
 } // end namespace llvm
 
 #endif // LLVM_IR_OPTBISECT_H

llvm/lib/IR/OptBisect.cpp

 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <limits>
 
 using namespace llvm;
 
 static cl::opt<int> OptBisectLimit("opt-bisect-limit", cl::Hidden,
-                                   cl::init(std::numeric_limits<int>::max()),
-                                   cl::Optional,
+                                   cl::init(OptBisect::Disabled), cl::Optional,
+                                   cl::cb<void, int>([](int Limit) {
+                                     llvm::OptBisector->setLimit(Limit);
+                                   }),
                                    cl::desc("Maximum optimization to perform"));
 
-OptBisect::OptBisect() : OptPassGate() {
-  BisectEnabled = OptBisectLimit != std::numeric_limits<int>::max();
-}
-
 static void printPassMessage(const StringRef &Name, int PassNum,
                              StringRef TargetDesc, bool Running) {
   StringRef Status = Running ? "" : "NOT ";
   errs() << "BISECT: " << Status << "running pass "
          << "(" << PassNum << ") " << Name << " on " << TargetDesc << "\n";
 }
 
 bool OptBisect::shouldRunPass(const Pass *P, StringRef IRDescription) {
-  assert(BisectEnabled);
+  assert(isEnabled());
 
   return checkPass(P->getPassName(), IRDescription);
 }
 
 bool OptBisect::checkPass(const StringRef PassName,
                           const StringRef TargetDesc) {
-  assert(BisectEnabled);
+  assert(isEnabled());
 
   int CurBisectNum = ++LastBisectNum;
-  bool ShouldRun = (OptBisectLimit == -1 || CurBisectNum <= OptBisectLimit);
+  bool ShouldRun = (BisectLimit == -1 || CurBisectNum <= BisectLimit);
   printPassMessage(PassName, CurBisectNum, TargetDesc, ShouldRun);
   return ShouldRun;
 }
 
+const int OptBisect::Disabled;
+
 ManagedStatic<OptBisect> llvm::OptBisector;