[PATCH] D15679: [FIX] Schedule generation PR25879

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

thanks for the detailed comments. They clearly help to both review the new code and to understand more of the existing code. I have a couple of follow up questions inline.

Best,
Tobias


================
Comment at: include/polly/ScopInfo.h:1410
@@ +1409,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?

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. By walking over the AST in reverse-post-order we ensure which condition precisely?

Also, can we state something about the loops processed in a region. Do we assume reducible control flow? Can/do we exploit the additional properties this gives us?

================
Comment at: include/polly/ScopInfo.h:1426
@@ +1425,3 @@
+  /// LoopSchedules map contains only one valid mapping, namely for the loop
+  /// surrounding the SCoP (or nullptr if none).
+  ///
----------------
Nice. This comment clearly helps me to better understand the algorithm used. Now I need to relate the actual implementation to the above description.

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? 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.
```
void buildSchedule(
      Region *R, 
      DenseMap<Loop *, std::pair<isl_schedule *, unsigned>> &LoopSchedules) {

}
```

to

```
isl_schedule *buildSchedule(Region *R) {
  DenseMap<Loop *, std::pair<isl_schedule *, unsigned>>
    &LocalLoopSchedules; ...
}
```

================
Comment at: lib/Analysis/ScopInfo.cpp:3448
@@ -3443,3 +3447,3 @@
     return;
   }
 
----------------
This comment clarifies a question I was wondering about before. For me this would become even more clear if we split off the non-recursive from the recursive part of the code. To try how if this actually works, I implemented such a patch ({F1284089}). If you agree this is useful I could commit it ahead of time.





================
Comment at: lib/Analysis/ScopInfo.cpp:3458
@@ +3457,3 @@
+  // the iterator (if any) to guarantee progress. If it is not set we first try
+  // the next queded sub-region/blocks.
+  //
----------------
queued

================
Comment at: lib/Analysis/ScopInfo.cpp:3493
@@ -3448,1 +3492,3 @@
+      LoopStack.push_back(L);
+    }
 
----------------
These different cases, iterators and lists look  confusing to me. I wonder if this code  would look more clear, if we would just add all RNs into a work list, to which we append RNs that need to be delayed? Moving all RNs to a work-list clearly slows down the common case, but I have doubts we see this in any profile.

================
Comment at: test/ScopInfo/wrong_schedule.ll:67
@@ +66,2 @@
+  ret void
+}
----------------
This patch still needs some simplifications (either manually or you could use my bugpoint patch).


http://reviews.llvm.org/D15679