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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added a comment.

This is definitely an improvement and I'm glad to see this change, but please see my inline comments.



================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:190
+static bool isLexicographicallyPositive(std::vector<char> &DV) {
+  for (unsigned Level = 0; Level < DV.size(); ++Level) {
+    unsigned char Direction = DV[Level];
----------------
Here we only check if direction is either `<` or `>`, but it could be `S`, which is a `non-"="` direction. I think we should return false if we encounter an `S` in this loop, or only iterate if it's not `=` or `I`.


================
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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137461