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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added a comment.

> 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

It would not be correct to ignore scalar dependencies, as they carry dependencies across all iterations of a loop, but in cases where the direction at the unrolled level is exactly EQ (eg in sub_sub_eq) we can assume safety without considering the inner levels. The reason is that if the unrolled level is exactly EQ, it will become LT (or GT) after unrolling which causes the dependencies in the inner loops to become non-fusion-preventing because the memory accesses would be disjoint. On the other hand, for cases like sub_sub_less, the dependence carried by the outer loop is LT but the dependence carried by the inner loop is scalar, which implies a lexicographical negativity. You get this check for free in your suggested implementation above because, by design, the direction bits in the DependenceInfo are all set when we have a scalar dependence, and `preservesForwardDependence` returns false causing the test to identify this case as illegal. We can add an explicit check for isScalar to be more clear, but I think just checking for the direction bits is simpler and good enough.

I don't think we need `preservesNonCarriedDependence` but we'd need a test for the EQ case, so I'd suggest removing `preservesNonCarriedDependence` and replacing `checkDependency` with the following which works well on all the tests and avoids the inexplicable skipping of scalar dependencies.

  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 the distance carried by the unrolled loop is 0, then after unrolling
    // that distance will become non-zero resulting in non-overlapping accesses in
    // the inner loops.
    if (UnrollDir == Dependence::DVEntry::EQ)
      return true;
  
    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;
  
    return true;
  }

We should also add a test for the following illegal case involving scalar dependence at the outer level:

  // loop k
  //   loop i
  //     loop j
  //       A(i-1, j) = A(i, j)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76132