[PATCH] D20770: [GSoC 2016][Polly][Refactor] Decouple SCoP building logic from pass

[Johannes Doerfert via llvm-commits]

jdoerfert added a comment.

I added some comments and change requests. Additionally, could you use "arc diff" to create a revision or alternatively add "-U99999" to the "diff" options such that we get context between the chunks that were changes? Thanks!


================
Comment at: include/polly/ScopInfo.h:2478
@@ +2477,3 @@
+  /// SESE-Region.
+  void createScopInfo(Region *R, AssumptionCache &AC);
+
----------------
I wondered why this method takes arguments already passed to the constructor and I needed to check the code. Two things:
  1) Please add a field for the AssumptionCache reference.
  2) The createScopInfo() function should be either:
     a) private and called "initialize" or "init"
     b) inlined into the constructor

================
Comment at: include/polly/ScopInfo.h:2482
@@ -2474,1 +2481,3 @@
+
+  virtual void print(raw_ostream &O, const Module *M = nullptr) const;
 
----------------
Remove the virtual please.

================
Comment at: include/polly/ScopInfo.h:2498
@@ +2497,3 @@
+  /// @brief The ScopInfo pointer which is used to construct a Scop.
+  std::unique_ptr<ScopInfo> SI;
+
----------------
I don't know why you need a unique_ptr here but I don't care to much about it.

================
Comment at: include/polly/ScopInfo.h:2510
@@ +2509,3 @@
+  /// @return Return ScopInfo object for the current Region.
+  ScopInfo &getScopInfo() { return *SI; }
+  const ScopInfo &getScopInfo() const { return *SI; }
----------------
I would not return a reference here because there is no way to know that SI is actually not null. Please return the SI pointer.

================
Comment at: lib/Analysis/ScopInfo.cpp:4869
@@ +4868,3 @@
+
+  if (!SD.isMaxRegionInScop(*R))
+    return;
----------------
can you move this check to the runOnRegion method below?

================
Comment at: lib/Analysis/ScopPass.cpp:23
@@ -22,3 +22,3 @@
 
-  if ((S = getAnalysis<ScopInfo>().getScop()))
+  if ((S = getAnalysis<ScopInfoRegionPass>().getScopInfo().getScop()))
     return runOnScop(*S);
----------------
I would change the interface such that getScopInfo might return a nullptr. At the moment I don't care if ScopInfo.getScop() returns a pointer or a reference but the latter might be possible.


http://reviews.llvm.org/D20770