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

[Johannes Doerfert via llvm-commits]

jdoerfert added a comment.

Some more comments from my side, but mostly style and simplification remarks/questions.


================
Comment at: lib/Transform/ScheduleOptimizer.cpp:166
@@ +165,3 @@
+  auto Dims = isl_set_dim(IsolateDomain, isl_dim_set);
+  auto IsolateRange =
+      isl_set_project_out(isl_set_copy(IsolateDomain), isl_dim_set, 0, Dims);
----------------
isn't this equivalent to isl_set_params(IsolateDomain)? If not can sb explain me the difference or maybe show me an example where it is not.

================
Comment at: lib/Transform/ScheduleOptimizer.cpp:174
@@ +173,3 @@
+  IsolateRelation = isl_map_intersect_domain(IsolateRelation, IsolateDomain);
+  IsolateRelation = isl_map_intersect_range(IsolateRelation, IsolateRange);
+  IsolateRelation = isl_map_move_dims(IsolateRelation, isl_dim_out, 0,
----------------
Can't we write all of the above as:
    auto *IsolateRelation = isl_map_from_domain(IsolateDomain);


Btw. please add the * to the auto types as it helps to get at least that part of information and is consistent with the rest of Polly.

================
Comment at: lib/Transform/ScheduleOptimizer.cpp:210
@@ +209,3 @@
+  ExtentConstraints = isl_set_drop_constraints_involving_dims(
+      ExtentConstraints, isl_dim_param, 0, DimsParam);
+  ExtentConstraints =
----------------
As Tobias once told me, drop_constraints_* is a dangerous function and it is usally not what we want. I do not fully understand why it is needed here but maybe we can use project_out or nothing?

================
Comment at: lib/Transform/ScheduleOptimizer.cpp:228
@@ +227,3 @@
+///             that contains a vector loop.
+
+static __isl_give isl_schedule_node *
----------------
This line is to much though ;)

================
Comment at: lib/Transform/ScheduleOptimizer.cpp:229
@@ +228,3 @@
+
+static __isl_give isl_schedule_node *
+isolateFullPartialTiles(__isl_take isl_schedule_node *Node) {
----------------
Can we make this a (static) member of the optimizer class? I would probably use it without the scheduling pass at some point.

================
Comment at: test/ScheduleOptimizer/full_partial_tile_separation.ll:21
@@ +20,3 @@
+; CHECK:               } else if (32 * c1 + 3 >= nj)
+; CHECK:                 for (int c5 = 0; c5 <= min(31, nk - 32 * c2 - 1); c5 += 1)
+; CHECK:                   #pragma simd
----------------
Out of curiosity, why do we have 3 cases? The first and second seem clear but I don't understand the third.


http://reviews.llvm.org/D13779