[PATCH] D32539: [Polly][NewPM][WIP] Add a ScopPassManager

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

I don't feel confident enough to evaluate the design. I don't know the new pass manager well enough nor its design pattern. so atm I can only give a very peripheral review.

`ScopAnalysisManagerFunctionProxy` and `FunctionToScopPassAdaptor` made me think about Java for some reason :). It feels like over-engineering, but I cannot claim that it isn't the way it is supposed to be done.



================
Comment at: include/polly/ScopInfo.h:2129
 
+  const StringRef getName() const { return name; }
+
----------------
There is already `getNameStr`. Why the need for another one?


================
Comment at: include/polly/ScopPass.h:9-10
 //===----------------------------------------------------------------------===//
-//
-// This file defines the ScopPass class.  ScopPasses are just RegionPasses,
-// except they operate on Polly IR (Scop and ScopStmt) built by ScopInfo Pass.
-// Because they operate on Polly IR, not the LLVM IR, ScopPasses are not allowed
-// to modify the LLVM IR. Due to this limitation, the ScopPass class takes
-// care of declaring that no LLVM passes are invalidated.
+// // This file defines the ScopPass class.  ScopPasses are just RegionPasses,
+// // except they operate on Polly IR (Scop and ScopStmt) built by ScopInfo
+// Pass. Because they operate on Polly IR, not the LLVM IR, ScopPasses are not
----------------
double `//`?


================
Comment at: include/polly/ScopPass.h:36
+
+namespace llvm {
+using polly::Scop;
----------------
Is it necessary to have this in the llvm namespace? Normally we should stay in our Polly namespace.

If it is necessary, doesn't it mean this should be moved to LLVM (respectively that the new pass manager is not designed for extensibility)?


================
Comment at: lib/Analysis/ScopInfo.cpp:3216
            ScopDetection::DetectionContext &DC)
-    : SE(&ScalarEvolution), R(R), IsOptimized(false),
-      HasSingleExitEdge(R.getExitingBlock()), HasErrorBlock(false),
-      MaxLoopDepth(0), CopyStmtsNum(0), DC(DC),
+    : SE(&ScalarEvolution), R(R), name("Scop for region " + R.getNameStr()),
+      IsOptimized(false), HasSingleExitEdge(R.getExitingBlock()),
----------------
This is more a description than a name.


================
Comment at: lib/Analysis/ScopPass.cpp:17
 
+#include <llvm/Analysis/AssumptionCache.h>
+
----------------
Independent of what makes more sense: We use quotes to inlcude llvm headers everwhere else.


Repository:
  rL LLVM

https://reviews.llvm.org/D32539