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

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

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

don't need this

3

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 ↗

(On Diff #311357)

82 ↗

(On Diff #311357)

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 ↗

(On Diff #311357)

llvm/lib/IR/OptBisect.cpp

58 ↗

(On Diff #311357)

newline at end of file?

llvm/lib/Passes/StandardInstrumentations.cpp

28 ↗

(On Diff #311357)

is this include necessary?

[swamulism]

Address comments

[swamulism]

Update comment strings

clang/lib/CodeGen/BackendUtil.cpp

   SI.registerCallbacks(PIC);
   PassBuilder PB(CodeGenOpts.DebugPassManager, TM.get(), PTO, PGOOpt, &PIC);
 
+  // Sets legacy pass manager OptBisect to the same one as npm so passes are properly skipped
+  TheModule->getContext().setOptPassGate(SI.getOptBisect());
+
   // Attempt to load pass plugins and register their callbacks with PB.
   for (auto &PluginFN : CodeGenOpts.PassPlugins) {
     auto PassPlugin = PassPlugin::Load(PluginFN);

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

+// Test NPM with -O1/opt-bisect
+//
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -O1 -fexperimental-new-pass-manager %s -fdebug-pass-manager -mllvm -opt-bisect-limit=-1 -emit-llvm -o /dev/null 2>&1 | FileCheck %s
+
+// CHECK: BISECT: running pass (1)
+// CHECK-NOT: BISECT: running pass (1)
+
+int func(int a) { return a; }

llvm/include/llvm/Passes/StandardInstrumentations.h

   void registerCallbacks(PassInstrumentationCallbacks &PIC);
 
   TimePassesHandler &getTimePasses() { return TimePasses; }
+  OptBisectInstrumentation &getOptBisect() { return OptBisect; }
 };
 
 extern template class ChangeReporter<std::string>;