[PATCH] D15679: [FIX] Schedule generation PR25879

[Johannes Doerfert via llvm-commits]

jdoerfert marked 2 inline comments as done.

================
Comment at: include/polly/ScopInfo.h:1414
@@ +1413,3 @@
+  /// Schedule generation traverses the SCoP region recursively in reverse
+  /// post-order (wrt. sub-regions). Whenever a loop header is encountered, the
+  /// corresponding loop is added to the @p LoopStack. This is needed since we
----------------
> Could you add one more sentence to clarify for the reader how the region tree traversal works on an abstract level?

I do not follow completely. What abstract level except "recursively in reverse post-order wrt. sub-regions" is there? 

> To my understanding for each call of buildSchedule we walk over a given region @R. @R consists of RegionNodes that are either BBs or itself non-trivial) Regions, which are all connected through control flow possibly containing loops.
Yes.

> By walking over the AST in reverse-post-order we ensure which condition precisely?
That we visit predecessors of a block before the block itself (same as for domain generation). This is necessary since we can then add the schedule for a loop or block in sequence to what we already have and do not need to find a place where to insert it.

> Also, can we state something about the loops processed in a region. 
I do not understand. What should I state about the loops?

> Do we assume reducible control flow? 
LoopInfo does not work on irreducible control => we will currently crash or do undefined things on irreducible control. If LoopInfo would work (in some sence) this code would probably work too (but I do not give a gurarantee as it heavily depends on the way irreducible control loops are modeled.).  

> Can/do we exploit the additional properties this gives us?
None crossed my mind so far but I do not know if there are none.

================
Comment at: include/polly/ScopInfo.h:1430
@@ +1429,3 @@
+  /// LoopSchedules map contains only one valid mapping, namely for the loop
+  /// surrounding the SCoP (or nullptr if none).
+  ///
----------------
> One thing I am not fully sure how it works is the LoopSchedules map (and the new LoopStack). Can we give an invariant on the state of these when a recursive call to buildSchedule returns?
Mh, invariants I can think of:
  - LoopStack, contains loops that are contained in each other, hence LoopStack[i]->getParentLoop() == LoopStack[i+1] (if there are at least i+1 elements).
  - If a loop was never in the LoopStack it is not mapped in the LoopSchedules map (except the loop that surrounds the SCoP, or nullptr if none).
  - If a loop is in the LoopStack it isl_schedule in the LoopSchedules is valid and the number of visited blocks in that loop is less than the number of blocks.
  - If a loop leaves the LoopStack its mapping in the LoopSchedules becomes invalid, thus the isl_schedule ptr is dangling and the second component (number of visited blocks) is equal to the number of blocks in the loop.

I can add them if it helps.

> Specifically, why do we need to return a full map of schedules? Will there possibly be always exactly one schedule in the map that is returned, e.g. the one for the current region R? Or is there an inherent reason (besides irreducible control flow) that prevents us from using function local state.
There is always "one active loop", thus one could rewrite it somehow to only pass the information for that one. However, we still need to pass the current schedule for that active loop as well as the number of visited blocks to the recursive calls and we need to update them during the recursion. One local map + std::pair<isl_schedule, unsigned>& argument should do the trick but I do not see how this is more efficent or easier.

================
Comment at: lib/Analysis/ScopInfo.cpp:3438
@@ -3437,3 +3437,3 @@
     return;
   }
 
----------------
Done.

================
Comment at: lib/Analysis/ScopInfo.cpp:3498
@@ -3454,1 +3497,3 @@
+      LoopStack.push_back(L);
+    }
 
----------------
But a workl list doesn't magically remove the different cases and iterators. The same logic is needed (if it is even that simple to decide if we need to delay a region node in case we do not yet build the schedule).


http://reviews.llvm.org/D15679