Page MenuHomePhabricator

Enable opt-bisect for the new pass manager
Needs ReviewPublic

Authored by cuviper on Sep 18 2020, 3:08 PM.

Details

Summary

This instruments a before-pass callback using the existing OptBisect
class to decide if new passes should be skipped. Passes that force
isRequired will still run anyway.

The test case is resurrected from r267022, an early version of D19172
that had new pass manager support (later reverted and redone without).

Diff Detail

Event Timeline

cuviper created this revision.Sep 18 2020, 3:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2020, 3:08 PM
cuviper requested review of this revision.Sep 18 2020, 3:08 PM

One of the issues raised in http://lists.llvm.org/pipermail/llvm-dev/2018-September/126477.html was that we'd have to somehow thread the -opt-bisect-limit through two different pass managers. If you look in BackendUtil.cpp's EmitAssemblyHelper::EmitAssemblyWithNewPassManager(), there's a NPM for the optimization pipeline, then a legacy PM for the codegen pipeline. OptBisect.rst says that you can use -opt-bisect-limit through clang -mllvm -opt-bisect-limit=, and I don't think that would properly work with this implementation since the codegen pipeline would restart -opt-bisect-limit.

I guess there aren't any tests for that though, since all references to -opt-bisect-limit are in llvm/test and use opt/llc.

llvm/lib/Passes/StandardInstrumentations.cpp
339–341

I don't think the NPM has a concept of a Region pass, so this shouldn't be necessary.

cuviper updated this revision to Diff 293787.Sep 23 2020, 10:09 AM

Dropped Region from NPM opt-bisect

Hmm, thanks for those pointers. To combine opt-bisect from the two pass managers, maybe there can be an explicit call to copy stateful instrumentation? I think it helps that they are not interleaved, so we could do MPM.run(), then somehow SI.copy_to(CodeGenPasses), then CodeGenPasses.run(). I still need to read that thread too...

LTOBackend may also need to deal with this, but I don't know if flags like opt-bisect apply there.

Hmm, thanks for those pointers. To combine opt-bisect from the two pass managers, maybe there can be an explicit call to copy stateful instrumentation? I think it helps that they are not interleaved, so we could do MPM.run(), then somehow SI.copy_to(CodeGenPasses), then CodeGenPasses.run(). I still need to read that thread too...

Yeah that makes sense. Or maybe have some global counter, since this is mostly a debugging thing, although that seems fairly hacky.

LTOBackend may also need to deal with this, but I don't know if flags like opt-bisect apply there.

Not sure...
I suppose we can think about interactions with the legacy PM in a later patch, this on its own is an improvement.

Another issue is opt-bisect interacting with required passes. Currently it seems that it'll print out "NOT running pass ..." on a required pass but still run it.
PassInstrumentation::runBeforePass() will check all instrumentations to see if it should skip the pass, but still override that decision if the pass is required.
Before, I had wanted to let the callback know if the pass was required, but at that point just overriding the decision in runBeforePass() worked. The logging seems pretty important here for user understandability.
Thoughts?

llvm/include/llvm/Passes/StandardInstrumentations.h
73

remove?
(I deleted the default constructors above just now)