[PATCH] D12499: [WIP] Replace ScalarEvolution based domain generation

[Tobias Grosser via llvm-commits]

grosser accepted this revision.
grosser added a comment.
This revision is now accepted and ready to land.

Hi Johannes,

this looks good for me so far.


================
Comment at: include/polly/ScopInfo.h:559
@@ -558,3 +557,1 @@
-  void addLoopBoundsToDomain(TempScop &tempScop);
-  void addLoopTripCountToDomain(const Loop *L);
   void buildDomain(TempScop &tempScop, const Region &CurRegion);
----------------
It seems you could drop some parts of TempScopInfo now, as they now become unused/dead-code.

================
Comment at: include/polly/ScopInfo.h:912
@@ +911,3 @@
+  ///
+  /// @param LI The LoopInfo analysis to argue about the number of iterators.
+  /// @param SD The ScopDetection analysis to identify non-affine sub-regions.
----------------
argue? This verb does not seem to make sense in this context.

================
Comment at: lib/Analysis/ScopInfo.cpp:817
@@ +816,3 @@
+/// @brief Add @p bset to the set @p User if @p bset is unbounded.
+static isl_stat collectUnboundedParts(__isl_take isl_basic_set *bset,
+                                      void *User) {
----------------
bset, maybe uppercase?

================
Comment at: lib/Analysis/ScopInfo.cpp:828
@@ +827,3 @@
+/// @brief Return @p S without basic sets that are unbounded in dimension @p
+/// Dim.
+static __isl_give std::pair<isl_set *, isl_set *>
----------------
Please comment the contents of the return value.

================
Comment at: lib/Analysis/ScopInfo.cpp:831
@@ +830,3 @@
+isolateBoundedParts(__isl_take isl_set *S, unsigned Dim) {
+  for (unsigned u = 0, e = isl_set_n_dim(S); u < e; u++)
+    S = isl_set_lower_bound_si(S, isl_dim_set, u, 0);
----------------
Uppercase iterators?

================
Comment at: lib/Analysis/ScopInfo.cpp:841
@@ +840,3 @@
+  OnlyDimS =
+      isl_set_remove_dims(OnlyDimS, isl_dim_set, Dim + 1, NumDimsS - Dim - 1);
+
----------------
Just removing dimensions is always fishy as this is just an operation of the internal set representation, so bad things can happen. I am not yet sure why this
is needed at all, but if it indeed is maybe you wanna use projection.

================
Comment at: lib/Analysis/ScopInfo.cpp:845
@@ +844,3 @@
+  // interested in
+  // them.
+  OnlyDimS = isl_set_insert_dims(OnlyDimS, isl_dim_param, 0, Dim);
----------------
Unnecessary line break

================
Comment at: lib/Analysis/ScopInfo.cpp:857
@@ +856,3 @@
+  isl_set_foreach_basic_set(OnlyDimS, collectUnboundedParts, &BoundedParts);
+  isl_set_free(OnlyDimS);
+
----------------
Maybe extract out a function that collects the bounded parts of an isl_set.

================
Comment at: lib/Analysis/ScopInfo.cpp:1705
@@ +1704,3 @@
+  // we will take the back edge, and then apply these restrictions to the
+  // header. Later we propagate them through the whole loop.
+
----------------
Please clarify that this propagation is not done later in this function itself, but is expected to be done afterwards (by another funciton) after this function has finished.

Also, the name of this function is misleading. Maybe call it addLoopBoundsToHeaderDomains

================
Comment at: lib/Analysis/ScopInfo.cpp:1729
@@ +1728,3 @@
+
+      SmallVector<isl_set *, 2> ConditionSets;
+      int idx = BI->getSuccessor(0) != HeaderBB;
----------------
Why an empty line?

================
Comment at: lib/Analysis/ScopInfo.cpp:1753
@@ +1752,3 @@
+          isl_set_remove_dims(BackedgeCondition, isl_dim_set, LoopDepth + 1,
+                              LatchLoopDepth - LoopDepth);
+
----------------
Please use project instead of remove_dims, as this  does not depend on the representation of the isl_set. 


http://reviews.llvm.org/D12499