[PATCH] D15679: [FIX] Schedule generation PR25879

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

thanks for the quick reply


================
Comment at: lib/Analysis/ScopInfo.cpp:3472
@@ -3439,1 +3471,3 @@
+      LoopStack.push_back(L);
+    }
 
----------------
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.



http://reviews.llvm.org/D15679