[PATCH] D108298: [NPM] Print '-passes' compatible string for built pipeline.

[Markus Lavin via Phabricator via llvm-commits]

markus added inline comments.


================
Comment at: llvm/include/llvm/IR/PassManager.h:1319
+                std::function<StringRef(StringRef)> MapClassName2PassName) {
+    OS << "invalidate<all>";
+  }
----------------
aeubanks wrote:
> markus wrote:
> > aeubanks wrote:
> > > does this not work with the default logic?
> > Apparently it does since `PassRegistry.def` contains 
> > ```
> >  MODULE_PASS("invalidate<all>", InvalidateAllAnalysesPass())
> > ```
> > So the implementation in `PassInfoMixin` handles it.
> > But this is different from the other `invalidate<>` though so IMHO it seems a bit nicer to be more explicit about it as there is a specific `InvalidateAllAnalysesPass` struct for it.
> to me having a separate `InvalidateAllAnalysesPass` is more of a reason to keep the default logic
Alright, I will adjust. In the end it does not really matter much though as long as we have regression tests to make sure that it works (which we do).


================
Comment at: llvm/include/llvm/IR/PassManagerInternal.h:98
+
+  void printPipelineParams(raw_ostream &OS) override {
+    Pass.printPipelineParams(OS);
----------------
aeubanks wrote:
> markus wrote:
> > aeubanks wrote:
> > > can this be part of `printPipeline`?
> > I don't think so. It needs to be separate so that it can be overridden separately in the Pass classes.
> I don't understand
> 
> why can't we for example have
> ```
> void SimplifyCFGPass::printPipeline(raw_ostream &OS) {
>   OS << name();
>   OS << "<";
>   OS << "bonus-inst-threshold=" << Options.BonusInstThreshold << ";";
>   OS << (Options.ForwardSwitchCondToPhi ? "" : "no-") << "forward-switch-cond;";
>   OS << (Options.ConvertSwitchToLookupTable ? "" : "no-")
>      << "switch-to-lookup;";
>   OS << (Options.NeedCanonicalLoop ? "" : "no-") << "keep-loops;";
>   OS << (Options.HoistCommonInsts ? "" : "no-") << "hoist-common-insts;";
>   OS << (Options.SinkCommonInsts ? "" : "no-") << "sink-common-insts";
>   OS << ">";
> }
> ```
Ah, now I understand what you meant. Yes I agree that looks better.


================
Comment at: llvm/test/Other/new-pm-print-pipeline.ll:3
+
+; RUN: opt -S -o /dev/null /dev/null -print-pipeline-passes -passes='verify,function(adce),function(simplifycfg<bonus-inst-threshold=123;no-forward-switch-cond;switch-to-lookup;keep-loops;no-hoist-common-insts;sink-common-insts>),verify' | FileCheck %s --match-full-lines --check-prefixes=CHECK-0
+; CHECK-0: verify,verify,function(adce),function(simplifycfg<bonus-inst-threshold=123;no-forward-switch-cond;switch-to-lookup;keep-loops;no-hoist-common-insts;sink-common-insts>),verify,verify,print
----------------
aeubanks wrote:
> aeubanks wrote:
> > aeubanks wrote:
> > > `-disable-output`
> > `< %s` seems nicer
> wait `-disable-output` shouldn't be necessary right? since we're not actually producing IR
If we don't use it then either the `print` pass or the `BitcodeWriterPass` (that is lacking a pass-name) will be part of the pipeline. So keeping this option makes the tests a bit cleaner I think.


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

https://reviews.llvm.org/D108298