[PATCH] [TODO] Add early exits for SCoPs we did not optimize

[Johannes Doerfert]

For me the compile time regressions seem like jitter (all < 0.001 sec) but the improvemeents don't (a lot > 0.01 sec). I don't know about the runtime effects, but except 2, all of the regressions seem like jittter too (again < 0.001 sec). I will look into flops or just run it 15 times in  isolation to get more confidence in the results.


================
Comment at: lib/Analysis/ScopInfo.cpp:699
@@ -698,3 +698,2 @@
   isl_space *Space = isl_space_set_alloc(getIslCtx(), 0, NbScatteringDims);
-  Space = isl_space_set_tuple_name(Space, isl_dim_out, "scattering");
 
----------------
grosser wrote:
> Why did you drop the tuple name that identifies the 'scattering'? This seems unrelated to the patch and causes a large number of seemingly unrelated test case changes. Was this intended?
It would have made my "isIdentitySchedule" check more complicated if we keep it but I'm fine with keeping it in the end (even though I do not see any benefit of the word "scattering" in the early representation of the scattering as we loose it after isl optimization anyway)

================
Comment at: lib/Analysis/ScopInfo.cpp:1516
@@ -1515,3 +1514,1 @@
-  DropDimMap = isl_map_set_tuple_id(
-      DropDimMap, isl_dim_out, isl_map_get_tuple_id(DropDimMap, isl_dim_in));
   for (auto *S : *this) {
----------------
grosser wrote:
> Again, this seems unrelated?
This was implied by the decision to remove it (see above)

================
Comment at: lib/CodeGen/IslAst.cpp:368
@@ -359,1 +367,3 @@
+    return;
+
   isl_ctx *Ctx = S->getIslCtx();
----------------
grosser wrote:
> Nice. It may even be worth outlining this cost estimation into its own function to clarify that we plan to extend/improve it.
That's true.

================
Comment at: lib/Transform/ScheduleOptimizer.cpp:560
@@ +559,3 @@
+  // Check if the scheduler actually performed some transformation.
+  // TODO: This is not the best way to check this.
+  bool changed = false;
----------------
grosser wrote:
> Maybe put this code into its own function?
We should try to come up with a better way to check this. I believe this doesn't catch all the cases and it should be unnecessary to iterate over the statements (maybe we can traverse the schedule map).

================
Comment at: lib/Transform/ScheduleOptimizer.cpp:578
@@ +577,3 @@
+    isl_map *OldStmtSchedule = Stmt->getScattering();
+    changed = !isl_map_is_subset(NewStmtSchedule, OldStmtSchedule);
+    isl_map_free(OldStmtSchedule);
----------------
grosser wrote:
> It seems here you check if the new schedule is a subset of the old schedule. This is a very simple heuristic and probably does not fire in a lot of cases (which is fine for me, as we can refine it later).
> 
> Instead of looping over all Stmts, you could possibly implement this by calling S.getSchedule() and comparing the result with ScheduleMap.
> 
> An alternative to the approach you are taking would be to assume we did not do anything, but just mark the scop as optimized if we clearly did something useful. This could e.g. be that we were able to perform tiling. I kind of like this idea, as it would make Polly be more conservative in terms of not changing existing code only if it is clear we can clearly improve performance.
I'm open for suggestions here and think we can improve this step by step. (I put the TODO above to make it clear I know this is not really a good way to check this).

http://reviews.llvm.org/D7254

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/