[PATCH] D15679: [FIX] Schedule generation PR25879

[Tobias Grosser via llvm-commits]

grosser added a comment.

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.

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?

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?

  mapToDimension(__isl_take isl_union_set *Domain, int N) {                        
    assert(N > 0);                                                                 
                                                                                   
    if (!Domain)                                                                   
      return nullptr;                                                              
                                                                                   
    assert(!isl_union_set_is_empty(Domain));                                       
                                                                                   
    struct MapToDimensionDataTy Data;                                              
                                                                                   
    auto *Space = isl_union_set_get_space(Domain);                                 
    auto *PwAff = isl_union_pw_multi_aff_empty(Space);                             
                                                                                   
    Data = {N, PwAff};                                                             
                                                                                   
    auto Res = isl_union_set_foreach_set(Domain, &mapToDimension_AddSet, &Data);   
                                                                                   
    assert(Res == isl_stat_ok);                                                    
                                                                                   
    isl_union_set_free(Domain);                                                    
    return isl_multi_union_pw_aff_from_union_pw_multi_aff(Data.Res);               
  }   

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

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?

Maybe you can consider these questions when documenting the code. Thank you.


http://reviews.llvm.org/D15679