[PATCH] D105006: [NewPM] Handle passes with params in -print-before/-print-after

[Yuanfang Chen via Phabricator via llvm-commits]

ychen added inline comments.


================
Comment at: llvm/lib/IR/PassInstrumentation.cpp:22
                                                       StringRef PassName) {
-  ClassToPassName[ClassName] = PassName.str();
+  if (ClassToPassName[ClassName].empty())
+    ClassToPassName[ClassName] = PassName.str();
----------------
bjope wrote:
> 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.
> 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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105006