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

[Evgeniy via Phabricator via llvm-commits]

ebrevnov marked an inline comment as done.
ebrevnov 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);
----------------
aeubanks wrote:
> 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
Ough... fixed.


================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:771
+  OptPassGate &PassGate = Context.getOptPassGate();
+  if (!PassGate.isEnabled())
+    return true;
----------------
aeubanks wrote:
> 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)
I thought getting module from "IR" slows down things.... looks like that's something else.
Ok I've moved isEnabled to registerCallbacks. 

> I don't think it's reasonable to change the gate while passes are running
I don't think either but current API allows doing that...IMHO, we better change the API to prohibit such scenario.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137149