[PATCH] D15679: [FIX] Schedule generation PR25879

[Tobias Grosser via llvm-commits]

grosser added a comment.

In http://reviews.llvm.org/D15679#315101, @jdoerfert wrote:

> In http://reviews.llvm.org/D15679#314985, @grosser wrote:
>
> > For reference test/ScopInfo/multiple_exiting_blocks_two_loop.ll is a test case that contains a scop with a loop that does not have a corresponding region.
> >
> > Johannes, I looked through the code and I have some difficulties to get a picture of what the exiting code does both in detail but also on a high-level. I started to think through it, but if you could add some documentation to the current code that would clearly help.
>
>
> I can do that.


Thank you.

> > Some questions I have:

> 

> > 

> 

> > What is LSchedulePair.second/NumVisited used for? It seems when the condition NumVisited == L->getNumBlocks() holds you assume all basic blocks of the loop have been processed. Then, you can start to walk up the loop tree and add the other loop dimensions?

> 

> 

> Wait. This was in there for months and is core part of the schedule generation. Maybe We should talk about two different things here. The existing code and what I propose to change to fix it. Apparently neither is clear.


Right. Let's start with the old code, then the new code is most likely a lot easier to understand.

> The second component of the schedule pair counts how many blocks of a loop have been visited. If this number is equal to the number of blocks in the loop we add this loop to the schedule tree. This is similar to what we did before (in the old schedule generation) but extended to the case that the loop is not perfectely covered by regions.

> 

> > I tried to understand the implementation of mapToDomain, which is simple in general but seems to do some additional stuff for certain corner cases that do not occur in our tests. Is the following a equivalent implementation? Under which conditions can Domain become 'NULL' (I checked and it indeed becomes NULL)? I removed the check for isl_union_set_is_empty, as this never happens in any test case and instead assert. Is this correct?

> 

> 

> 

> 

> 1. Except the first conditional the mapToDimensions was implemented by you.

> 2. In the current implementation N should always be positive.

> 3. I am not 100% sure why the empty check was in there... maybe it is not needed anymore, maybe it occures somewhere in lnt. If lnt is green i'm fine with you removing that check.


LNT is green. I committed an improved version of this change in r256208.

> > How does the content of LoopSchedules change throught the walk? Can you give some invariant. E.g. when buildSchedule returns, what does LoopSchedules contain?

> 

> 

> Initially, it is empty. Nothing is in there and LSchedulePair for every loop will be <nullptr, 0>. Then we will build up isl_schedules and add the number of blocks visited to change both components until all blocks of a loop have been visited and its schedule is integrated into the parent loops schedule. During the recursive construction LoopSchedules is constructed not completely but almost "bottom up". While the schedules for the inner-most loops are constructed the schedule of outer loops can be constructed as well but inner loops should be traversed completely before outer ones and their schedule will then be integrated into the outer loop.


OK, now I see. Adding this in the documentation would clearly be useful.

> > It seems LSchedule can become nullptr, as the following patch asserts:

> 

> > 

> 

> >   @@ -3464,6 +3468,7 @@ void Scop::buildSchedule(

> 

> >    

> 

> >        isl_schedule *LSchedule = LSchedulePair.first;

> 

> >        unsigned NumVisited = LSchedulePair.second;

> 

> >   +    assert(LSchedule);

> 

> >        while (L && NumVisited == L->getNumBlocks()) {

> 

> >   

> 

> > 

> 

> > 

> 

> > This is a little surprising. What does it mean if LSchedule is nullptr? When does this happen?

> 

> 

> Initially (when a loop is discovered) this is a nullptr as there is no schedule for the loop yet. This nullptr is combined "in sequence" with some real schedule and then stored back in the first component of LSchedule.


OK. I re-factored the code slightly in r256209 to not perform any computation on the nullptr. This makes the code more
readable for me.

Best,
Tobias


http://reviews.llvm.org/D15679