[PATCH] D45210: [New-PM] Lift Scop Pipeline to CGSCC-level

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Philip and me talked about the idea keeping both Function- and CGSCC-level passes around. Philipp was convinced that there was no benefit. For reference, here were my concerns (I don't know the new pas manager that well):

1. A function-level transformation pass wants to use Polly analysis. Since function passes can be run in any order by the pass manager, it does not necessarily correspond to the CGSCC (reverse? post-order) expected by the Polly passes. Callees have been already modified by this hypothetical pass after being analyzed by Polly.

2. A JIT or similar (e.g. clang runs a few LLVM passes directly after codegening a function) may not have added or know all callers/callees to the llvm::Module yet, i.e. the call graph is not complete/available.

[Rational: There must be some reason for why not all LLVM function passes are CGSCC-passes]

Other than a few nitpicks: LGTM.



================
Comment at: include/polly/ScopPass.h:278
   ScopPassT Pass;
-}; // namespace polly
+};
 
----------------
philip.pfaffe wrote:
> unrelated change
The comments are added by `clang-format`, `make check-polly` probably already complained about format changes.


================
Comment at: lib/Analysis/ScopInfo.cpp:5150-5159
+    Function &F = SCCNode.getFunction();
+    auto &FAM =
+        CGAM.getResult<FunctionAnalysisManagerCGSCCProxy>(SCC, CG).getManager();
+    auto &SI = FAM.getResult<ScopInfoAnalysis>(F);
+    for (auto &It : reverse(SI)) {
+      if (It.second)
+        It.second->print(Stream, PollyPrintInstructions);
----------------
[Style] You could just call the other overload than to duplicate its code.


================
Comment at: lib/Analysis/ScopPass.cpp:149
   }
-
   return false; // This proxy is still valid
----------------
[Nit] Unnecessary change


================
Comment at: lib/Support/RegisterPasses.cpp:702
+  PM.addPass(createCGSCCToScopPassAdaptor(std::move(SPM)));
+  // FIXME Regarding VerifyEachPass: There is no CGSCC-level verifier pass
+  MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(PM)));
----------------
The meaning of the FIXME comment is not clear without the deleted code.
Suggestion:
```
// FIXME: When VerifyEachPass is enabled, we are supposed to add a VerifierPass to the pipeline here. However, there is no CGSCC-level verifier pass.
```


Repository:
  rPLO Polly

https://reviews.llvm.org/D45210