[PATCH] D20962: [Polly][GSoC 2016]New function pass ScopInfoWrapperPass

[Johannes Doerfert via llvm-commits]

jdoerfert added inline comments.

================
Comment at: include/polly/ScopInfo.h:2517
@@ +2516,3 @@
+/// This pass will maintain a map of Region to its Scop object for all the
+/// top level regions present in the function.
+/// This pass is designed to be used by other larger passes such as
----------------
Why top level regions? I don't think that is true.

================
Comment at: include/polly/ScopInfo.h:2519
@@ +2518,3 @@
+/// This pass is designed to be used by other larger passes such as
+/// PolyhedralInfo function pass as currently it is not feasible to schedula a
+/// region pass from a function pass in LLVM.
----------------
what is a "larger pass"? Maybe just replace this by sth like:
  This pass is an alternative to the ScopInfoRegionPass in order to avoid a region pass manager.

================
Comment at: include/polly/ScopInfo.h:2531
@@ +2530,3 @@
+  ///        Polly IR of static control part
+  RegionToScopMapTy regionToScopMap;
+
----------------
```
RegionToScopMap;
```

================
Comment at: include/polly/ScopInfo.h:2540
@@ +2539,3 @@
+  /// @brief Get the ScopInfo object for the given Region
+  Scop *getScop(Region *R) {
+    auto it = regionToScopMap.find(R);
----------------
```
Scop *getScop(Region *R) const {
  return RegionToScopMap.lookup();
}
```

================
Comment at: include/polly/ScopInfo.h:2549
@@ +2548,3 @@
+  const Scop *getScop(Region *R) const {
+    auto it = regionToScopMap.find(R);
+    if (it != regionToScopMap.end())
----------------
You don't need this function, the above should be enough.

================
Comment at: include/polly/ScopInfo.h:2563
@@ +2562,3 @@
+  void releaseMemory() override {
+    for (auto &it : regionToScopMap) {
+      it.second.reset();
----------------
What's the point of the unique_ptr if we treat them as raw_ptrs anyway?

If I am not mistaken you can remove this loop and only clear the map. The unique_ptrs should automatically call delete on the Scops they point to.

================
Comment at: lib/Analysis/ScopInfo.cpp:4876
@@ -4875,1 +4875,3 @@
+/// Implementation of ScopInfoRegionPass begins here.
+//
 void ScopInfoRegionPass::getAnalysisUsage(AnalysisUsage &AU) const {
----------------
I don't like such lines, if nobody is in favour of them I would say we do not introduce them.

================
Comment at: lib/Analysis/ScopInfo.cpp:4937
@@ +4936,3 @@
+/// Implementation of ScopInfoWrapperPass begins here.
+//
+void ScopInfoWrapperPass::getAnalysisUsage(AnalysisUsage &AU) const {
----------------
See above.

================
Comment at: lib/Analysis/ScopInfo.cpp:4962
@@ +4961,3 @@
+  /// function.
+  for (ScopDetection::iterator I = SD.begin(), E = SD.end(); I != E; ++I) {
+    R = const_cast<Region *>(*I);
----------------
```
for (auto R : SD) { ... }
```

================
Comment at: lib/Analysis/ScopInfo.cpp:4968
@@ +4967,3 @@
+    ScopBuilder SB(R, AC, AA, DL, DT, LI, SD, SE);
+    regionToScopMap.insert(std::make_pair(R, SB.getScop()));
+  }
----------------
```
RegionToScopMap[R] = SB.getScop();
```

================
Comment at: lib/Analysis/ScopInfo.cpp:4974
@@ +4973,3 @@
+void ScopInfoWrapperPass::print(raw_ostream &OS, const Module *) const {
+  for (auto &it : regionToScopMap) {
+    if (it.second)
----------------
```
It
```

Please take another look at the naming convention we use.


================
Comment at: lib/Analysis/ScopInfo.cpp:4987
@@ +4986,3 @@
+INITIALIZE_PASS_BEGIN(
+    ScopInfoWrapperPass, "polly-scops-all",
+    "Polly - Create polyhedral description of all Scops of a function", false,
----------------
polly-scops-all sounds more like "detect unprofitable scops" than "detect scops in a function scope". I would call it -polly-scops-function or -polly-function-scops or somehing similar..


http://reviews.llvm.org/D20962