[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:81
 public:
-  OptBisectInstrumentation() = default;
+  OptPassGateInstrumentation(LLVMContext &Context) : Context(Context){};
+  bool shouldRun(StringRef PassName, Any IR);
----------------
ebrevnov wrote:
> aeubanks wrote:
> > clang-format?
> clang-format works this way for me...what problem do you see here?
there should be a space in `Context(Context){};`

I was confused why clang-format wasn't adding the space, eventually figured out it's the unnecessary semicolon haha


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:771
+  OptPassGate &PassGate = Context.getOptPassGate();
+  if (!PassGate.isEnabled())
+    return true;
----------------
ebrevnov wrote:
> aeubanks wrote:
> > this should be in `OptPassGateInstrumentation::registerCallbacks` gating `registerShouldRunOptionalPassCallback` to avoid an extra callback per pass run
> PassGate can be set/remove/changed to LLVMContext at any time using LLVMContext::setOptPassGate() API. Thus it would be invalid to "cache" the state in the registerCallbacks.
again, this causes compile time issues: https://llvm-compile-time-tracker.com/compare.php?from=66b830889d9bc4bfc58aa1ea47d1b3a7cbee7c5c&to=c0e4d153eecc11f1bed41f242e72197c0d5c7baa&stat=instructions:u

I don't think it's reasonable to change the gate while passes are running

(also I had to update BackendUtil.cpp in clang to get it building, you can see that in the precommit bots)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149