[PATCH] D76132: [LoopUnrollAndJam] Changed safety checks to consider more than 2-levels loop nest.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Given that the DependencyAnalysis result has to be interpreted different than I initially though, I came up with a new legality check.

Since DA returns (lexical) forward and backwards dependency in a single result, both have to be checked. There is also a symmetry between them as the alternative would be to invoke `->depends(Src,Dst)` and well as `->depends(Dst,Src)` and check them separately.

  static bool preservesForwardDependence(Instruction *Src, Instruction *Dst,
                                         unsigned UnrollLevel, unsigned JamLevel,
                                         bool Sequentialized, Dependence *D) {
    // UnrollLevel might carry the dependency Src --> Dst
    // Does a different loop after unrolling?
    for (unsigned d = UnrollLevel + 1; d <= JamLevel; ++d) {
      auto JammedDir = D->getDirection(d);
      if (JammedDir == Dependence::DVEntry::LT)
        return true;
  
      if (JammedDir & Dependence::DVEntry::GT)
        return false;
    }
  
    return true;
  }
  
  static bool preservesBackwardDependence(Instruction *Src, Instruction *Dst,
                                          unsigned UnrollLevel, unsigned JamLevel,
                                          bool Sequentialized, Dependence *D) {
    // UnrollLevel might carry the dependency Dst --> Src
    for (unsigned d = UnrollLevel + 1; d <= JamLevel; ++d) {
      auto JammedDir = D->getDirection(d);
      if (JammedDir == Dependence::DVEntry::GT)
        return true;
  
      if (JammedDir & Dependence::DVEntry::LT)
        return false;
    }
  
    // Backward dependencies are only preserved if not interleaved.
    return Sequentialized;
  }
  
  /// Also a forward-dependency, but not carried by UnrollLoop.
  static bool preservesNonCarriedDependence(Instruction *Src, Instruction *Dst,
                                            unsigned UnrollLevel,
                                            unsigned JamLevel,
                                            bool Sequentialized, Dependence *D) {
    // There might be dependency Src --> Dst that is not carried by UnrollLoop.
    for (unsigned d = UnrollLevel + 1; d <= JamLevel; ++d) {
      // TODO: Justify this; without it, sub_sub_eq fails
      if (D->isScalar(d))
        continue;
  
      auto JammedDir = D->getDirection(d);
      if (JammedDir == Dependence::DVEntry::LT)
        return true;
  
      if (JammedDir & Dependence::DVEntry::GT)
        return false;
    }
  
    return true;
  }
  
  static bool checkDependency(Instruction *Src, Instruction *Dst,
                              unsigned UnrollLevel, unsigned JamLevel,
                              bool Sequentialized, DependenceInfo &DI) {
    assert(UnrollLevel <= JamLevel);
  
    if (Src == Dst)
      return true;
    if (isa<LoadInst>(Src) && isa<LoadInst>(Dst))
      return true;
  
    std::unique_ptr<Dependence> D = DI.depends(Src, Dst, true);
    if (!D)
      return true;
    assert(D->isOrdered() && "Expected an output, flow or anti dep.");
  
    // Quick bail-out.
    if (D->isConfused())
      return false;
  
    for (unsigned d = 1; d < UnrollLevel; ++d) {
      // Insert comment justifying this here
      if (!(D->getDirection(d) & Dependence::DVEntry::EQ))
        return true;
    }
  
    auto UnrollDir = D->getDirection(UnrollLevel);
    if (UnrollDir & Dependence::DVEntry::LT &&
        !preservesForwardDependence(Src, Dst, UnrollLevel, JamLevel,
                                    Sequentialized, D.get()))
      return false;
  
    if (UnrollDir & Dependence::DVEntry::GT &&
        !preservesBackwardDependence(Src, Dst, UnrollLevel, JamLevel,
                                     Sequentialized, D.get()))
      return false;
  
    if (UnrollDir & Dependence::DVEntry::EQ &&
        !preservesNonCarriedDependence(Src, Dst, UnrollLevel, JamLevel,
                                       Sequentialized, D.get()))
      return false;
  
    return true;
  }

This seem to work nicely with that checks, but I am not sure whether it is correct to ignore `isScalar` in the EQ case and why.  It seems obvious that the `sub_sub_eq` test case can be unroll-and-jammed. Bit if we add the same skip to `preservesForwardDependence`, the test case `sub_sub_less` is unroll-and-jammed which it must not,



================
Comment at: llvm/test/Transforms/LoopUnrollAndJam/dependencies.ll:363
 ; CHECK-NOT: %j.1 = phi
-define void @sub_sub_less(i32* noalias nocapture %A, i32 %N, i32* noalias nocapture readonly %B) {
 entry:
----------------
Whitney wrote:
> dmgreen wrote:
> > Whitney wrote:
> > > This test was orignally testing
> > > ```
> > > for i
> > >   for j
> > >     A[i]
> > >     A[i-1]
> > > ```
> > > which should be **safe** to unroll and jam. 
> > > 
> > > I think it actually want to test the code for sub with sub with the dependence distance of the inner loop is less.
> > > ```
> > > for i
> > >   for j
> > >     A[i][j]
> > >     A[i+1][j-1]
> > > ```
> > Hmm. I don't remember what this was trying to test. It feels like a very long time ago now.
> > 
> > Thanks for splitting the new tests out. More are always a good thing.
> For sure. I guess is hard to speculate what it was trying to test now.  
This is not safe to unroll-and-jam. For %N == 2 the excution sequence is (Sa being the first access in the loop body, Sb the second)
```
Sa(0,0): A[0]
Sb(0,0): A[-1]
Sa(0,1): A[0]
Sb(0,1): A[-1]
Sa(1,0): A[1]
Sb(1,0): A[0]
Sa(1,1): A[1]
Sb(1,1): A[0]
```

After unroll-and-jam by 2:
```
Sa(0,0): A[0]
Sb(0,0): A[-1]
Sa(1,0): A[1]
Sb(1,0): A[0]
Sa(0,1): A[0]
Sb(0,1): A[-1]
Sa(1,1): A[1]
Sb(1,1): A[0]
```

That is, the dependency chain `Sa(0,0)->Sa(0,1)->Sb(1,0)->Sb(1,1)` has become `Sa(0,0)->Sb(1,0)->Sa(0,1)->Sb(1,1)` and therefore has been violated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76132/new/

https://reviews.llvm.org/D76132