[PATCH] D13779: [Polly] Full/partial tile separation for vectorization

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Roman,

the patch looks good so far. I just have a set of minor documentation comments.

Best,
Tobias


================
Comment at: lib/Transform/ScheduleOptimizer.cpp:165
@@ +164,3 @@
+getIsolateOptions(__isl_take isl_set *IsolateDomain) {
+  auto Dims = isl_set_dim(IsolateDomain, isl_dim_set);
+  auto IsolateRange =
----------------
Maybe add a brief comment what the function is doing.

================
Comment at: lib/Transform/ScheduleOptimizer.cpp:181
@@ +180,3 @@
+}
+
+static __isl_give isl_union_set *getAtomicOptions(__isl_take isl_space *Space) {
----------------
Maybe add a brief comment what the function is doing.

================
Comment at: lib/Transform/ScheduleOptimizer.cpp:193
@@ +192,3 @@
+/// 2. Extend it to a set, which has exactly VectorWidth iterations for
+///    an every prefix from the previous step.
+/// 3. Subtract loop domain from it, project out the vector loop dimension and
----------------
"an every" ?  

================
Comment at: lib/Transform/ScheduleOptimizer.cpp:202
@@ +201,3 @@
+
+static __isl_give isl_set *getPrefixes(__isl_take isl_set *ScheduleRange) {
+  auto Dims = isl_set_dim(ScheduleRange, isl_dim_set);
----------------
No empty line in between.

Also it is unclear what prefixes you are talking about both in the function name and in the @brief header. Maybe call it 'getPartialTilePrefixes()' and describe early on in the comment what kind of prefixes you are calculating.

================
Comment at: lib/Transform/ScheduleOptimizer.cpp:221-222
@@ +220,4 @@
+
+/// @brief Isolate a set of prefixes.
+///
+/// This set should ensure that any prefix from the set has exactly
----------------
Again, 'prefixes' is rather generic. Maybe use 'partial tile prefixes'?

================
Comment at: lib/Transform/ScheduleOptimizer.cpp:232
@@ +231,3 @@
+  if (isl_schedule_node_get_type(Node) != isl_schedule_node_band)
+    return Node;
+  Node = isl_schedule_node_child(Node, 0);
----------------
Can we make this an assert?


http://reviews.llvm.org/D13779