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

[Tobias Grosser]

Hi Johannes,

as discussed before, I really like the idea. I added some minor comments inline.

Regarding the performance numbers, it is very nice to see that your patch reduces compile time for several of the TSVC benchmarks.I also see two compile and execution time regressions of around 50% with your patch. The compile time regression is possibly noise, but the execution time regression is surprising. Did you had a chance to see what we changed in Misc/flops to get the nice speedup that with you patch applied is lost again?

Cheers,
Tobias


================
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");
 
----------------
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?

================
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) {
----------------
Again, this seems unrelated?

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

================
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;
----------------
Maybe put this code into its own function?

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

http://reviews.llvm.org/D7254

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