[LoopInterchange] Redundant isLexicographicallyPositive check

[sjoerdmeijer]

Two checks are performed to see if a direction vector is lexigraphically positive: before and after the interchange transformation. A direction vector has to be positive, so the check before transformation is more of a sanity check to see if the dependence analysis is correct, but it is redundant with the sanity check that is performed after the interchange permute.

We can't assert that the direction vector before is lexicographically positive, because the "don't know" elements are rightfully interpreted as not lexicographically positive. But if there's a "don't know" element, then the permuted direction vector will be rejected too, so the outcome is the same, so let's just skip the first check all together.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Sjoerd Meijer (sjoerdmeijer)

Changes

Two checks are performed to see if a direction vector is lexigraphically positive: before and after the interchange transformation. A direction vector has to be positive, so the check before transformation is more of a sanity check to see if the dependence analysis is correct, but it is redundant with the sanity check that is performed after the interchange permute.

We can't assert that the direction vector before is lexicographically positive, because the "don't know" elements are rightfully interpreted as not lexicographically positive. But if there's a "don't know" element, then the permuted direction vector will be rejected too, so the outcome is the same, so let's just skip the first check all together.

---

Full diff: https://github.com/llvm/llvm-project/pull/117343.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/LoopInterchange.cpp (+5-4)

diff --git a/llvm/lib/Transforms/Scalar/LoopInterchange.cpp b/llvm/lib/Transforms/Scalar/LoopInterchange.cpp
index a0c0080c0bda1c..6d08535a40115f 100644
--- a/llvm/lib/Transforms/Scalar/LoopInterchange.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopInterchange.cpp
@@ -204,11 +204,12 @@ static bool isLegalToInterChangeLoops(CharMatrix &DepMatrix,
   std::vector<char> Cur;
   // For each row check if it is valid to interchange.
   for (unsigned Row = 0; Row < NumRows; ++Row) {
-    // Create temporary DepVector check its lexicographical order
-    // before and after swapping OuterLoop vs InnerLoop
+    // A dependence vector has to be lexigraphically positive. We could assert
+    // that here, as a sanity check for the dependency analysis, but it can
+    // contain "don't know" elements ('*'), which will be rightfully
+    // interpreted as not lexicographical positive. So, skip that sanity check,
+    // it suffices just to check the interchanged direction vector.
     Cur = DepMatrix[Row];
-    if (!isLexicographicallyPositive(Cur))
-      return false;
     std::swap(Cur[InnerLoopId], Cur[OuterLoopId]);
     if (!isLexicographicallyPositive(Cur))
       return false;

[CongzheUalberta]

AFAIK, there was a reason for isLexicographicallyPositive(Cur) to be called before the swap. I did not recall explicitly but it likely has something to do with 'S' or '*' dependency. We had encountered a number of bugs due to the fact that 'S' and '*' not very easily dealt with in loop interchange.

If you remove the isLexicographicallyPositive() call here, likely bugs would appear. I probably do not have enough time digging into it at the moment, but you may refer to this patch (https://reviews.llvm.org/D137461) and also earlier patches and issues in loop interchange.

[sjoerdmeijer]

Thanks for that pointer.

I agree with the following from that phabricator review, that's what I am saying too:

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.

The impression that I get from that discussion and the mishandling of scalar dependencies, is that we are not really sure why we are doing things.

Anyway, I don't have too strong opinions on this, I still don't think it is necessary, but I will focus on #119345 first.

[kasuga-fj]

Hi, I found this PR by chance. I think isLexicographicallyPositive check is necessary before the interchange.

if there's a "don't know" element, then the permuted direction vector will be rejected too.

I think the following case is a counterexample to this.

#define N 4
int a[N][N];
void f() {
  for (int i = 0; i < N; i++) {
    for (int j = 0; j < N-1; j++) {
      a[j][N-1-i] += a[j+1][i];
    }
  }
}

Please see also: https://godbolt.org/z/roP9qc7PK

There is a direction vector [* <], which is interpreted as not lexicographically positive before the interchange, but positive after it.

If you want to reduce compilation time, I think one way would be to stop all the subsequent processes and exit when we find a direction vector with * on its first element. However, with improving the legality check, I believe some of these cases should be interchangeable in the future.