This is an archive of the discontinued LLVM Phabricator instance.

[NewPM] Handle passes with params in -print-before/-print-after
ClosedPublic

Authored by bjope on Jun 28 2021, 2:24 AM.

Details

Summary

To support options like -print-before=<pass> and -print-after=<pass>
the PassBuilder will register PassInstrumentation callbacks as well
as a mapping between internal pass class names and the pass names
used in those options (and other cmd line interfaces). But for
some reason all the passes that takes options where missing in those
maps, so for example "-print-after=loop-vectorize" didn't work.

This patch will add the missing entries by also taking care of
function and loop passes with params when setting up the class to
pass name maps.

One might notice that even with this patch it might be tricky to
know what pass name to use in options such as -print-after. This
because there only is a single mapping from class name to pass name,
while the PassRegistry currently is a bit messy as it sometimes
reuses the same class for different pass names (without using the
"pass with params" scheme, or the pass-name<variant> syntax).

It gets extra messy in some situations. For example the
MemorySanitizerPass can run like this (with debug and print-after)

opt -passes='kmsan' -print-after=msan-module -debug-only=msan

The 'kmsan' alias for 'msan<kernel>' is just confusing as one might
think that 'kmsan' is a separate pass (but the DEBUG_TYPE is still
just 'msan'). And since the module pass version of the pass adds
a mapping from 'MemorySanitizerPass' to 'msan-module' one need to
use 'msan-module' in the print-before and print-after options.

Diff Detail

Event Timeline

bjope created this revision.Jun 28 2021, 2:24 AM
bjope requested review of this revision.Jun 28 2021, 2:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2021, 2:24 AM
bjope added inline comments.Jun 28 2021, 2:37 AM
llvm/lib/IR/PassInstrumentation.cpp
22–23

By adding some printouts when this check fails, I get this (after also having applied D105007):

PassInstrumentation note: HWAddressSanitizerPass is already mapped to hwasan. It will not be mapped to hwasan<kernal;recover>
PassInstrumentation note: ModuleInlinerWrapperPass is already mapped to inliner-wrapper. It will not be mapped to inliner-wrapper-no-mandatory-first
PassInstrumentation note: ModuleInlinerWrapperPass is already mapped to inliner-wrapper. It will not be mapped to scc-oz-module-inliner
PassInstrumentation note: LoopExtractorPass is already mapped to loop-extract. It will not be mapped to loop-extract-single
PassInstrumentation note: ModuleAddressSanitizerPass is already mapped to asan-module. It will not be mapped to kasan-module
PassInstrumentation note: EarlyCSEPass is already mapped to early-cse. It will not be mapped to early-cse-memssa
PassInstrumentation note: EntryExitInstrumenterPass is already mapped to ee-instrument. It will not be mapped to post-inline-ee-instrument
PassInstrumentation note: LowerMatrixIntrinsicsPass is already mapped to lower-matrix-intrinsics. It will not be mapped to lower-matrix-intrinsics-minimal
PassInstrumentation note: AddressSanitizerPass is already mapped to asan. It will not be mapped to asan<kernel>
PassInstrumentation note: MemorySanitizerPass is already mapped to msan-module. It will not be mapped to msan
PassInstrumentation note: ThreadSanitizerPass is already mapped to tsan-module. It will not be mapped to tsan
PassInstrumentation note: MemorySanitizerPass is already mapped to msan-module. It will not be mapped to msan
PassInstrumentation note: SimplifyCFGPass is already mapped to simplifycfg. It will not be mapped to simplify-cfg
PassInstrumentation note: SimpleLoopUnswitchPass is already mapped to simple-loop-unswitch. It will not be mapped to unswitch

Not sure if we need to add a one-to-many map to handle some of the above. Or if for example early-cse-memssa should be called early-cse<memssa> to indicate that it is a specialized version of early-cse.

For passes like ee-instrument. and post-inline-ee-instrument it might be trickier. Should enabling print-after for one of those trigger IR printouts for both? I.e. should they be seen as two different passes, or parameterized versions of the same pass?

I also think it is just confusing to have both simplifycfg and simplify-cfg<>, as well as both simple-loop-unswitch and unswitch<>.

Not sure what to do with the sanitizer passes that both exist as module and function passes, using the same class name.

ychen added inline comments.Jun 28 2021, 11:20 AM
llvm/lib/IR/PassInstrumentation.cpp
22–23

By adding some printouts when this check fails, I get this (after also having applied D105007):

PassInstrumentation note: HWAddressSanitizerPass is already mapped to hwasan. It will not be mapped to hwasan<kernal;recover>
PassInstrumentation note: ModuleInlinerWrapperPass is already mapped to inliner-wrapper. It will not be mapped to inliner-wrapper-no-mandatory-first
PassInstrumentation note: ModuleInlinerWrapperPass is already mapped to inliner-wrapper. It will not be mapped to scc-oz-module-inliner
PassInstrumentation note: LoopExtractorPass is already mapped to loop-extract. It will not be mapped to loop-extract-single
PassInstrumentation note: ModuleAddressSanitizerPass is already mapped to asan-module. It will not be mapped to kasan-module
PassInstrumentation note: EarlyCSEPass is already mapped to early-cse. It will not be mapped to early-cse-memssa
PassInstrumentation note: EntryExitInstrumenterPass is already mapped to ee-instrument. It will not be mapped to post-inline-ee-instrument
PassInstrumentation note: LowerMatrixIntrinsicsPass is already mapped to lower-matrix-intrinsics. It will not be mapped to lower-matrix-intrinsics-minimal
PassInstrumentation note: AddressSanitizerPass is already mapped to asan. It will not be mapped to asan<kernel>
PassInstrumentation note: MemorySanitizerPass is already mapped to msan-module. It will not be mapped to msan
PassInstrumentation note: ThreadSanitizerPass is already mapped to tsan-module. It will not be mapped to tsan
PassInstrumentation note: MemorySanitizerPass is already mapped to msan-module. It will not be mapped to msan
PassInstrumentation note: SimplifyCFGPass is already mapped to simplifycfg. It will not be mapped to simplify-cfg
PassInstrumentation note: SimpleLoopUnswitchPass is already mapped to simple-loop-unswitch. It will not be mapped to unswitch

Not sure if we need to add a one-to-many map to handle some of the above. Or if for example early-cse-memssa should be called early-cse<memssa> to indicate that it is a specialized version of early-cse.

I think we really need to get rid of the global map. (D97722)

For passes like ee-instrument. and post-inline-ee-instrument it might be trickier. Should enabling print-after for one of those trigger IR printouts for both? I.e. should they be seen as two different passes, or parameterized versions of the same pass?

I think it should be "parameterized versions of the same pass" which is exactly what it is.

I also think it is just confusing to have both simplifycfg and simplify-cfg<>, as well as both simple-loop-unswitch and unswitch<>.

Agree. This is the same issue as "ee-instrument". We should have one name for a pass and using "<>" to pass parameters from commandline.

Not sure what to do with the sanitizer passes that both exist as module and function passes, using the same class name.

Maybe some mangling scheme, like "msan#m<>" for module pass, "msan#f<>" for function pass, and so on.

what's the actual use case for this? --print-before/after are hacky, there's usually a better way to accomplish something aside from --print-before/after

bjope added a comment.Jun 29 2021, 1:20 AM

what's the actual use case for this? --print-before/after are hacky, there's usually a better way to accomplish something aside from --print-before/after

I actually tried to use getPassNameForClassName for something else when I noticed that all passes with parameteras were missing in the map. I kind of wanted to get a listing of all the "pass names" that were used when for example running with "-passes='default<O3>' by letting the PrintPassInstrumentation print both the class name and pass name. Maybe that could be solved by not using class names in the PassInstrumentation/PassInstrumentationCallbacks in the first place. It kind of looks like someone has assumed that there is a one-to-one mapping between class names and pass names, but currently it is a one-to-many relation.

Nevertheless. The print-before and print-after options do exist, so I guess they should work also for the passes with params. If you want to remove them, then I think that is a different story. Someone has bothered to add support for those options in new-PM, so if you want to remove them you better discuss it with the ones who added the option. I'm just trying to fix problems I see with new-PM where the implementation is broken/incomplete.

ormris removed a subscriber: ormris.Jun 29 2021, 10:15 AM
bjope added a comment.Jul 5 2021, 2:21 AM

Seems like we agree that this only is a start (that we should follow up with fixes for other passes such as in D105007).

And I'm well aware that the PassRegistry might be refactored completely in the future (but for example D97722 isn't really moving forward). Fixing the current implementation like this might however show-case the need for a refactoring a bit more. And it also helps setting the requirement on the new solution (to avoid that we end up refactoring a broken implementation without also considering the currently missing use-cases).

So what is the verdict on this patch?

ychen added a comment.Jul 6 2021, 11:38 AM

Seems like we agree that this only is a start (that we should follow up with fixes for other passes such as in D105007).

And I'm well aware that the PassRegistry might be refactored completely in the future (but for example D97722 isn't really moving forward). Fixing the current implementation like this might however show-case the need for a refactoring a bit more. And it also helps setting the requirement on the new solution (to avoid that we end up refactoring a broken implementation without also considering the currently missing use-cases).

So what is the verdict on this patch?

Sounds reasonable to me.

llvm/test/Other/print-before-after.ll
39

XXXCHECK-UNSWITCH?

bjope added inline comments.Jul 6 2021, 3:34 PM
llvm/test/Other/print-before-after.ll
39

Those are the printouts that still are missing, as explained in the FIXME. The goal would be to enable those checks by removing the "XXX". But then we need to remove the "unswitch" alias for "simple-loop-unswitch".

ychen added inline comments.Jul 7 2021, 4:50 PM
llvm/test/Other/print-before-after.ll
39

I'd use CHECK-UNSWITCH-NOT. It is unusual to use unchecked labels.

bjope updated this revision to Diff 357203.Jul 8 2021, 6:21 AM

Updated test case after review comments.

ychen accepted this revision.Jul 8 2021, 10:40 AM

LGTM

This revision is now accepted and ready to land.Jul 8 2021, 10:40 AM