Make NPM OptBisectInstrumentation use global singleton OptBisect

[swamulism]

Summary

Currently there is an issue where the legacy pass manager uses a different OptBisect counter than the new pass manager.
This fix makes the npm OptBisectInstrumentation use the global OptBisect.

[aeubanks]

Thanks for looking into this!

Can you upload the diff with full context (e.g. use diff -U 9999 or use arcanist to upload)?

clang/lib/CodeGen/BackendUtil.cpp

1153 ↗

(On Diff #310386)

I took another look, if you look in LLVMContextImpl::getOptPassGate, it says the OptBisect instance must outlive the LLVMContext, which isn't true here. There's actually a global OptBisect there that we should use.

So I think we should go the opposite way. The NPM's StandardInstrumentations shouldn't contain its own OptBisect, it should inspect the IR it's receiving to get the IR's LLVMContext and call LLVMContext::getOptPassGate().

e.g. after unwrapping Any to Function *, F->getContext().getOptPassGate().

clang/test/CodeGen/new-pass-manager-O1-opt-bisect.c

1 ↗

(On Diff #310386)

Something like "Make sure opt-bisect works through both pass managers" is probably better.
Also the test name doesn't need "O1" in it, just "new-pass-manager-opt-bisect.c" is good

3 ↗

(On Diff #310386)

don't need this

3 ↗

(On Diff #310386)

does this actually run the codegen passes? I thought we needed -emit-obj.

[aeubanks]

Oh and in the description

This means that when using OptBisect under the NPM it runs twice when it should only run once.

doesn't really make sense. I'd say something like

e.g. -opt-bisect-limit=1 will run both the first optional optimization pass and the first optional codegen pass, when really it should skip the codegen pass

[swamulism]

Updating to set npm OptBisect to use global singleton OptBisect instead

[swamulism]

Thank you for the comments and code review!

[aeubanks]

llvm/include/llvm/IR/OptBisect.h

58

83

should probably mention that this is so that multiple pass managers don't need to coordinate their uses of OptBisect

llvm/lib/IR/LLVMContextImpl.cpp

222–223

llvm/lib/IR/OptBisect.cpp

58

newline at end of file?

llvm/lib/Passes/StandardInstrumentations.cpp

27

is this include necessary?

[swamulism]

Address comments

[swamulism]

Update comment strings

clang/test/CodeGen/new-pass-manager-opt-bisect.c

+// Make sure opt-bisect works through both pass managers
+//
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -O1 -fexperimental-new-pass-manager %s -mllvm -opt-bisect-limit=-1 -emit-obj -o /dev/null 2>&1 | FileCheck %s
+
+// CHECK: BISECT: running pass (1)
+// CHECK-NOT: BISECT: running pass (1)
+// Make sure that legacy pass manager is running
+// CHECK: Instruction Selection
+
+int func(int a) { return a; }

llvm/include/llvm/IR/OptBisect.h

 /// This file declares the interface for bisecting optimizations.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_IR_OPTBISECT_H
 #define LLVM_IR_OPTBISECT_H
 
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/ManagedStatic.h"
 
 namespace llvm {
 
 class Pass;
 
 /// Extensions to this class implement mechanisms to disable passes and
 /// individual optimizations at compile time.
 class OptPassGate {
 public:
   virtual ~OptPassGate() = default;
 
   /// IRDescription is a textual description of the IR unit the pass is running
   /// over.
   virtual bool shouldRunPass(const Pass *P, StringRef IRDescription) {
     return true;
   }
 
-  /// isEnabled should return true before calling shouldRunPass
+  /// isEnabled() should return true before calling shouldRunPass().
   virtual bool isEnabled() const { return false; }
 };
 
 /// 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.
   ///
   /// Clients should not instantiate this class directly.  All access should go
   /// through LLVMContext.
   OptBisect();
 
   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; }
+
+  /// 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 shouldRunPass(const Pass *P, StringRef IRDescription) override;
-
-  /// isEnabled should return true before calling shouldRunPass
-  bool isEnabled() const override { return BisectEnabled; }
-
-protected:
   bool checkPass(const StringRef PassName, const StringRef TargetDesc);
 
 private:
   bool BisectEnabled = false;
   unsigned 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/include/llvm/Passes/StandardInstrumentations.h

public:
   OptNoneInstrumentation(bool DebugLogging) : DebugLogging(DebugLogging) {}
   void registerCallbacks(PassInstrumentationCallbacks &PIC);
 
 private:
   bool DebugLogging;
   bool shouldRun(StringRef PassID, Any IR);
 };
 
-class OptBisectInstrumentation : public OptBisect {
+class OptBisectInstrumentation {
 public:
   OptBisectInstrumentation() {}
   void registerCallbacks(PassInstrumentationCallbacks &PIC);
 };
 
 // Debug logging for transformation and analysis passes.
 class PrintPassInstrumentation {
 public:

llvm/lib/IR/LLVMContextImpl.cpp

 
 void LLVMContextImpl::getSyncScopeNames(
     SmallVectorImpl<StringRef> &SSNs) const {
   SSNs.resize(SSC.size());
   for (const auto &SSE : SSC)
     SSNs[SSE.second] = SSE.first();
 }
 
-/// Singleton instance of the OptBisect class.
-///
-/// This singleton is accessed via the LLVMContext::getOptPassGate() function.
-/// It provides 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.
-///
-/// Even if multiple LLVMContext objects are created, they will all return the
-/// same instance of OptBisect in order to provide a single bisect count.  Any
-/// code that uses the OptBisect object should be serialized when bisection is
-/// enabled in order to enable a consistent bisect count.
-static ManagedStatic<OptBisect> OptBisector;
-
+/// Gets the OptPassGate for this LLVMContextImpl, which defaults to the
+/// singleton OptBisect if not explicitly set.
 OptPassGate &LLVMContextImpl::getOptPassGate() const {
   if (!OPG)
     OPG = &(*OptBisector);
   return *OPG;
 }
 
 void LLVMContextImpl::setOptPassGate(OptPassGate& OPG) {
   this->OPG = &OPG;
 }

llvm/lib/IR/OptBisect.cpp

bool OptBisect::checkPass(const StringRef PassName,
                           const StringRef TargetDesc) {
   assert(BisectEnabled);
 
   int CurBisectNum = ++LastBisectNum;
   bool ShouldRun = (OptBisectLimit == -1 || CurBisectNum <= OptBisectLimit);
   printPassMessage(PassName, CurBisectNum, TargetDesc, ShouldRun);
   return ShouldRun;
 }
+
+ManagedStatic<OptBisect> llvm::OptBisector;

llvm/lib/Passes/StandardInstrumentations.cpp

 #include "llvm/Analysis/CallGraphSCCPass.h"
 #include "llvm/Analysis/LazyCallGraph.h"
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassInstrumentation.h"
 #include "llvm/IR/PrintPasses.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/raw_ostream.h"
 #include <unordered_set>
 #include <vector>
 
 using namespace llvm;
 
if (any_isa<const Loop *>(IR)) {
     return "loop";
   }
 
   llvm_unreachable("Unknown wrapped IR type");
 }
 
 void OptBisectInstrumentation::registerCallbacks(
     PassInstrumentationCallbacks &PIC) {
-  if (!isEnabled())
+  if (!OptBisector->isEnabled())
     return;
-
-  PIC.registerShouldRunOptionalPassCallback([this](StringRef PassID, Any IR) {
-    return isIgnored(PassID) || checkPass(PassID, getBisectDescription(IR));
+  PIC.registerShouldRunOptionalPassCallback([](StringRef PassID, Any IR) {
+    return isIgnored(PassID) ||
+           OptBisector->checkPass(PassID, getBisectDescription(IR));
   });
 }
 
 void PrintPassInstrumentation::registerCallbacks(
     PassInstrumentationCallbacks &PIC) {
   if (!DebugLogging)
     return;