[PATCH] D20770: [GSoC 2016][Polly][Refactor] Decouple SCoP building logic from pass
Tobias Grosser via llvm-commits
llvm-commits at lists.llvm.org
Mon May 30 05:10:15 PDT 2016
grosser added a comment.
Hi Utpal,
thank you for looking into this! As Johannes said, the patch looks generally good. I mostly have minor comments.
Also, I would suggest to look after this commit into the memory management of ScopInfo/ScopBuilder. It seems unnecessary to keep it around after the scop was constructed just to keep a pointer to the scop. I would prefer to just have it return the scop object and (or a unique_ptr to it) and not keep any state after the scop has been constructed.
Best,
Tobias
================
Comment at: include/polly/ScopInfo.h:2250
@@ -2249,3 +2249,3 @@
/// @brief Build the Polly IR (Scop and ScopStmt) on a Region.
-class ScopInfo : public RegionPass {
//===-------------------------------------------------------------------===//
----------------
Would it make sense to call this ScopBuilder? This seems more descriptive.
You can probably do this in a follow-up commit.
================
Comment at: include/polly/ScopInfo.h:2476
@@ +2475,3 @@
+
+ void releaseMemory() { clear(); }
+
----------------
The releaseMemory method does not seem to be needed any more, no?
================
Comment at: include/polly/ScopInfo.h:2478
@@ -2474,1 +2477,3 @@
+
+ void print(raw_ostream &O, const Module *M = nullptr) const;
----------------
Why is a print method needed here? We could just get the scop and print it directly, no?
================
Comment at: lib/Analysis/ScopPass.cpp:23
@@ +22,3 @@
+ ScopInfo *SI = getAnalysis<ScopInfoRegionPass>().getScopInfo();
+ if (SI)
+ if ((S = SI->getScop()))
----------------
Johannes, I do not understand this comment/change? Why do we need to expose ScopInfo/ScopBuilder? Either getScop() returns a scop or a nullptr? That seems all the functionality we need, right?
http://reviews.llvm.org/D20770
More information about the llvm-commits
mailing list