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

[Tobias Grosser via llvm-commits]

Thanks Johannes for the comments. All remarks are clearly useful.

On 10/15/2015 11:27 PM, Johannes Doerfert wrote:
> 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.

Seems like, indeed.

> ================
> 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.

Good 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.

Isl is distinguishing four cases:

1) The part to isolate
2) The part before the isolated part
3) The part after the isolated part
4) The part that is executed if there is no isolated part

In this case  1, 3 & 4 are generated while 2 is empty and left out. isl 
should probably merge parts 3/4 or 2/4 if possible and beneficial, but
this optimization has not yet been implemented (I remember there were 
some issues). I gonna send a test case to isl-dev for Sven to have a look.

Best,
Tobias