[PATCH] D137103: [LegacyPM] Port example pass SimplifyCFG to new PM

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

In D137103#3897293 <https://reviews.llvm.org/D137103#3897293>, @speryt wrote:

> This is currently WIP patch, because I have some issues in understanding how exactly the porting should happen. I've been following @aeubanks recommendation from discourse <https://discourse.llvm.org/t/deprecation-of-enable-new-pm-option/65168/20?u=speryt>, but two main things that I found unclear so far are:
>
> 1. Should I retain legacy code, or can I safely drop it? In `Bye` pass I can see that legacy pass is still present, which for me makes little sense to copy for this pass. Especially because goal of this effort is to drop flag enabling run on legacy PM.

Yes we should drop everything specific to the legacy pass manager here.

> 1. Looking for example at `llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp` it seems that different approach has been used to add support for new PM, which is closer to what documentation <https://llvm.org/docs/NewPassManager.html> is showing, rather what is presented in `Bye` pass. Mainly in the are of registration of new pass as well as pass options, which are using special processing function as I understand. Which way is "the proper one"?

Since this is a plugin rather than a real in-tree pass that's always available, it doesn't use the same registration. https://reviews.llvm.org/D136626 updates new PM plugin docs.
I wouldn't worry about pass options, using a `cl::opt` is fine for now. We can make the pass option changes later if we want.



================
Comment at: llvm/examples/IRTransforms/SimplifyCFG.cpp:413
+
+    bool PassExecuted = false;
+
----------------
a little misleading, I'd say `MadeChange` instead


================
Comment at: llvm/examples/IRTransforms/SimplifyCFG.cpp:419
+    case V2: {
+  DominatorTree &DT = FAM.getResult<DominatorTreeAnalysis>(F);
+      PassExecuted = doSimplify_v2(F, DT);
----------------
formatting


================
Comment at: llvm/examples/IRTransforms/SimplifyCFG.cpp:433
+  PreservedAnalyses PA;
+  PA.preserveSet<CFGAnalyses>();
+  return PA;
----------------
this pass definitely doesn't preserve the CFG, we can just `return PreservedAnalyses::none()`


================
Comment at: llvm/examples/IRTransforms/SimplifyCFG.cpp:443
+          [](PassBuilder &PB) {
+            PB.registerVectorizerStartEPCallback(
+                [](llvm::FunctionPassManager &PM, OptimizationLevel Level) {
----------------
I don't think the legacy pass does this, drop this part?


================
Comment at: llvm/examples/IRTransforms/SimplifyCFG.cpp:469
+llvmGetPassPluginInfo() {
+  return getSimplifyPluginInfo();
+}
----------------
`getSimplifyCFGPluginInfo`?


================
Comment at: llvm/examples/IRTransforms/SimplifyCFG.h:19
 
 FunctionPass *createSimplifyCFGPass();
 
----------------
speryt wrote:
> I couldn't find call to `createSimplifyCFGPass` anywhere. In other places (e.g. `llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp`)  similar function is actually used
> 
> ```
> // Public interface to the CFGSimplification pass
> FunctionPass *
> llvm::createCFGSimplificationPass(SimplifyCFGOptions Options,
>                                   std::function<bool(const Function &)> Ftor) {
>   return new CFGSimplifyPass(Options, std::move(Ftor));
> }
> ```
> 
> Do we really need this function here?
it was probably just an example on how you'd typically create a legacy pass. yes we should remove it and the `InitializePasses.cpp/h` in this folder


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137103