[PATCH] D137149: Use PassGate from LLVMContext if any otherwise global one
Evgeniy via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 22 22:14:01 PST 2022
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
More information about the llvm-commits
mailing list