This is an archive of the discontinued LLVM Phabricator instance.

Enable opt-bisect for the new pass manager
ClosedPublic

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

Details

Summary

This instruments a should-run-optional-pass callback using the existing
OptBisect class to decide if new passes should be skipped. Passes that
force isRequired never reach this at all, so they are not included in
"BISECT:" output nor its pass count.

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
566–568

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)

https://reviews.llvm.org/D90545 will make it so we don't even run this instrumentation on required passes, making it so we don't print "skipping pass" on a required pass. It also makes the opt-bisect counter not increment on passes that will run anyway, making it clearer what's going on. After that lands this should be good to go.

Great, thanks for the update! I will rebase after that lands.

That has landed, we should probably amend the test to make sure that it doesn't print "BISECT: (NOT) running pass" for required passes, e.g. verify

cuviper updated this revision to Diff 304003.Nov 9 2020, 3:43 PM

Rebase, and test that required VerifierPass is not bisected

cuviper edited the summary of this revision. (Show Details)Nov 9 2020, 3:45 PM
aeubanks accepted this revision.Nov 9 2020, 3:52 PM

Thanks for doing this!

This revision is now accepted and ready to land.Nov 9 2020, 3:52 PM
This revision was landed with ongoing or failed builds.Nov 9 2020, 3:58 PM
This revision was automatically updated to reflect the committed changes.
dmajor added a subscriber: dmajor.Nov 10 2020, 7:46 AM

I'm very happy to see this land, thank you for writing it!