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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/include/llvm/IR/PassManager.h:1319
+                std::function<StringRef(StringRef)> MapClassName2PassName) {
+    OS << "invalidate<all>";
+  }
----------------
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


================
Comment at: llvm/include/llvm/IR/PassManagerInternal.h:98
+
+  void printPipelineParams(raw_ostream &OS) override {
+    Pass.printPipelineParams(OS);
----------------
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 << ">";
}
```


================
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:
> > `-disable-output`
> `< %s` seems nicer
wait `-disable-output` shouldn't be necessary right? since we're not actually producing IR


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

https://reviews.llvm.org/D108298