[PATCH] D92897: Set legacy pass manager OptBisect to same as NPM OptBisect

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

Thanks for looking into this!

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



================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1153
+  // Sets legacy pass manager OptBisect to the same one as npm so passes are properly skipped
+  TheModule->getContext().setOptPassGate(SI.getOptBisect());
+
----------------
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()`.


================
Comment at: clang/test/CodeGen/new-pass-manager-O1-opt-bisect.c:1
+// Test NPM with -O1/opt-bisect
+//
----------------
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


================
Comment at: clang/test/CodeGen/new-pass-manager-O1-opt-bisect.c:3
+//
+// 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
+
----------------
don't need this


================
Comment at: clang/test/CodeGen/new-pass-manager-O1-opt-bisect.c:3
+//
+// 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
+
----------------
aeubanks wrote:
> don't need this
does this actually run the codegen passes? I thought we needed `-emit-obj`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92897/new/

https://reviews.llvm.org/D92897