[PATCH] D137461: [LoopInterchange] Refactor and rewrite validDepInterchange()

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

The two changes (`Direction == '*'` and checking the dep vector before doing the interchange) may add safety, but also cause tests to fail. @bmahjour suggested we could mark those as `XFAIL` and fix in a followup-patch, if not directly handled in this patch. Thing that I think are two strict:

1. I don't think the check before the interchange is necessary. Dependency vectors in the original code are in forward direction by definition.
2. The surrounding loops do not matter if the interchanged loops can still preserve any dependency. That is, the two innermost loop in a dependency vector `(* < <)` can be interchanged. Same argument that true dependency must have been forward in the original code. Think of the `*` being in another function that calls a function containing the `< <` loops. In that case, DependenceAnalysis would not even see that surrounding loops and would not have mattered. On the other side, the case `(< * *)` could still be interchanged because the dependency is known to be already carried by that surrounding loops and the two inner loops don't need to care.



================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:147-152
             if (Dir == Dependence::DVEntry::LT ||
                 Dir == Dependence::DVEntry::LE)
               Direction = '<';
             else if (Dir == Dependence::DVEntry::GT ||
                      Dir == Dependence::DVEntry::GE)
               Direction = '>';
----------------
`<=` and `>=` are handled the same way as `<` and `>` respectively. I think this is incorrect. 

Consider the loops i,j,k and the the dependency vector `(<,*,<=)`. i and k cannot be interchanged as it would yield `(<=,*,< )`: `<=` is not guaranteed to carry all the dependencies before hitting `*`. This code here handles is the same as `(<,*,<)` where i and k can be interchanged.
Alternatively, to have just neighboring loops, consider `(*,<=)` assuming we do not need to check the before-state that was suggested by @bmahjour .


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:194
+      return true;
+    if (Direction == '>')
       return false;
----------------
A dependency vector with a leading `*` (and maybe `S` as @bmahjour suggests) (such as `(* <)`, `(= * <)`, `(= = * <)` etc) would not be known to be lexicographically positive. Even if this would change the semantics, I think we should change this so match what is expected from a function called `isLexicographicallyPositive`.

I think this was missing to supersede the `*` logic below.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:213
+    // Create temporary DepVector and swap OuterLoop vs InnerLoop
+    std::vector<char> Cur = DepMatrix[Row];
+    std::swap(Cur[InnerLoopId], Cur[OuterLoopId]);
----------------
[suggestion] Consider moving the `std::vector<char>` declaration out of the loop so it does not need to be reallocated each time, and/or `SmallVector`


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:207-210
     char InnerDep = DepMatrix[Row][InnerLoopId];
     char OuterDep = DepMatrix[Row][OuterLoopId];
     if (InnerDep == '*' || OuterDep == '*')
       return false;
----------------
bmahjour wrote:
> Narutoworld wrote:
> > Meinersbur wrote:
> > > I think this logic here is also superseded by the new logic.
> > Hi @Meinersbur, Thanks for your comment.
> > 
> > The logic says if any dependency vector of InnerLoop or OuterLoop is unknown, it is not legal to do interchange.
> > And the function `isLexicographicallyPositive()` only checked the direction vector after potential interchage. 
> > 
> > Suppose we have a dependency direction vector (*, <, =) for 3 layers loop, i, j, k. and we want to check if we can interchange loop i, j.
> > 
> > The original code will prohibit interchange since `i-Dep is *`.
> > 
> > However, with `isLexicographicallyPositive()`, the temp dependency direction vector is (<,*,=), which makes it a valid interchange candidate.
> > Although I can add more codes to force the same logic, (checking the interchanged position has a * ), it contains logic beyond checking **Lexicographical Order** only. 
> > 
> > That is the reason why I would like to keep the original code.
> > 
> I prefer having `isLexicographicallyPositive()` handle all kinds of possible directions including `S` and `*`, this way it stands up to its name. We can check that a direction vector is positive before we swap two columns. If the direction vector is not positive to begin with, that means we couldn't correct it even with the normalization applied, so it must be treated as unsafe to interchange. If it passes the first test, we can then check if the direction vector is positive after doing the swap. This makes the logic simpler as it's all about whether an entry is isLexicographicallyPositive() or not, albeit we need to call the function twice for each row.
I do think interchanging the loop that results in a `(<, *, =)` vector is perfectly fine. This is because the original dependency will always be lexicographic positive because .. the instruction cannot have dependendent on another instruction that is executed in the future. The confused `*` state comes from the limitation from being represented in a dependence vector or limitations of DependenceAnalysis not understanding the dependency.

With moving `<` to the outer position, we know that dependence will be carried by that loop and the inner position dependency becomes irrelevant.

At the LoopWG call we discussed that we still could call `isLexicographicallyPositive` before the interchange to check that the dependency is understood before risting breaking things. I don't think it's necessary, but I don't mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137461