[PATCH] D15679: [FIX] Schedule generation PR25879

[Johannes Doerfert via llvm-commits]

jdoerfert added inline comments.

================
Comment at: lib/Analysis/ScopInfo.cpp:3472
@@ -3439,1 +3471,3 @@
+      LoopStack.push_back(L);
+    }
 
----------------
grosser wrote:
> jdoerfert wrote:
> > grosser wrote:
> > > This piece of code looks a little complex. At the very least it requires some in-source comments.
> > > 
> > > From my point of view, this code would possibly be easier to understand if we find a way to somehow (more explicitly) walk down the loop tree and just do the ReversePostOrderTraversal to process the statements within the loop bodies. We had such a tree walk already earlier, but it was removed when you generalized the schedule construction. Would adding back such a tree walk be difficult due to the generality of the CFGs we allow?
> > > 
> > > One of the reasons this might be difficult is if we aim to support irreducible control flow (http://llvm.org/PR25909). Due to LoopInfo working with natural loops which can not be formed from irregular control flow, we do not correctly model irregular control flow before and after this patch and I believe we should also not try to do so.  Do you have any intentions to support such control flow with your patch?
> > I could outline it and/or add a comment,  is that OK? I am unsure how your want to achieve a reverse post order traversal of all blocks while staying in loops first more explicitly. What we had used the Region::element_XXX() iterator. I was not aware of any guarantees on the traversal order when you use this iterator (it does not even have a comment). If __you know__ of any tree traversal that will
> >   1) traverse blocks in reverse post order
> >   2) traverse loops (even if they are not covered by a sub-region) first once the header was visisted.
> > we probably use it.
> > 
> > I am unsure why you talk about irreducible control flow now. The patches comes with an reducible CFG test case for which we generated a __plain wrong__ schedule. The irreducible case will be broken as long as LoopInfo does not offer information about a irreducible loop, there is nothing I can do about that in the ScopInfo anyway (except write an own loop info ...).
> > 
> > 
> I do not yet have a good idea regarding the tree traversal, but would like to think about it a little.
> 
> Hence, I try to understand how general the scops are that you expect to handle. Can you point me to a test case for "traverse loops (even if they are not covered by a sub-region)"?
> 
> I just talked about irreducible control flow to understand if you plan to support it. It seems we agree that we do not want to handle it (but somehow detect it in the scop detection). From this I conclude that we could theoretically walk over the loop info tree and then for each loop enumerate the basic block it contains (which might allow
> us to structure this without introducing a worker list). Now, the issue you point out is that we need to iterate over the blocks in a given order. I have no solution yet, but will think about it later today.
> 
> Documentation will clearly help me to understand this issue. Especially, if it would contain some difficult cases you thought about while implementing.
> 
Take your time. This is a bug that is in Polly but (for some reason) does not cause to much trouble.

Generally all CFGs should be fine __iff__ LoopInfo and ScalarEvolution play along. As LoopInfo *breaks* for irreducible CFGs and ScalarEvolution for some other cases (e.g., piecewiese AddRecs) we should not detetect them as SCoPs.

I think the example below should illustrate some interstring problems:


```

pre_header:
  store
  br loop_header

loop_header:
  store
  switch [loop_body1, loop_body2, loop_exit1, loop_exit2]

loop_body1:
  store
  br loop_header

loop_body2:
  store
  br loop_header

loop_exit1:
  store
  br after_loop

loop_exit2:
  store
  br after_loop

after_loop
  store
  br ...

```
We have to __guarantee__ the blocks are visited in the following order (/ means "or"):
  pre_header
  loop_header
  loop_body1/loop_body2         [not loop_exit1/loop_exit2]
  loop_body1/loop_body2         [not loop_exit1/loop_exit2]   
  loop_exit1/loop_exit2
  loop_exit1/loop_exit2
  after_loop

While reverse post order __can__ traverse the CFG that way, it is not unique and might choose:
  pre_header
  loop_header
  loop_body1/loop_body2         [not loop_exit1/loop_exit2]
  loop_exit1/loop_exit2         [BAD!]
  loop_body1/loop_body2
  loop_exit1/loop_exit2
  after_loop


http://reviews.llvm.org/D15679