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

[Tobias Grosser]

================
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");
 
----------------
jdoerfert wrote:
> 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)
Dropping 'scattering' as a tuple name may indeed be a good idea. If you  could commit this part separately, the test case noise would not obscure this patch.

================
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;
----------------
jdoerfert wrote:
> 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).
This identify check is indeed a very rough heuristic. Improving it by deriving the set of performed transformations by looking at the resulting schedules seems incredibly hard.

I see two alternative approaches:

- We compute properties (# of stride-one-accesses, ...)
- We keep track of optimizations we have successfully performed (tiling)

both require some thoughts and may not be needed for the initial version.

As said earlier, the current code that looks for subset schedules can be simplified by using S.getSchedule() and comparing the result with ScheduleMap.

================
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);
----------------
jdoerfert wrote:
> 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).
Maybe you can actually state in your todo what we do today and what could be done later.

// To understand if the schedule has been optimized we check if the schedule has
// changed at all.
// TODO: We can improve this by tracking if any necessarily beneficial 
// transformations have been performed. This can e.g. be tiling, loop interchange,
// or ...) We can track this either at the place where the transformation has been
// performed or, in case of automatic ILP based optimizations, by comparing
// (yet to be defined) performance metrics before/after the scheduling optimizer
// (e.g. number of stride-one accesses)

Also, I think it would be good to get a test case for our heursitic here, similar to how the loop vectorizer has test cases for its performance model:

Transforms/LoopVectorize/X86/uint64_to_fp64-cost-model.ll

For us a simple optimized/non-optimized is probably a good start,

http://reviews.llvm.org/D7254

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