[LoopInterchange] Prevent from undoing its own transformation

LoopInterchange uses the bubble-sort fashion algorithm to sort the loops
in a loop nest. For two loops A and B, it calls isProfitable(A, B) to
determine whether these loops should be exchanged. The result of
isProfitable(A, B) should be conservative, that is, it should return
true only when we are sure exchanging A and B will improve performance.
If it's not conservative enough, it's possible that a subsequent
isProfitable(B, A) will also return true, in which case LoopInterchange
will undo its previous transformation. To make isProfitable conservative
enough, isProfitable(B, A) must not return true if isProfitable(A, B)
returned true in the past. However, the current implementation can be in
such a situation.

isProfitable consists of several multiple rules, and they are applied
one by one. The first rule is isProfitablePerLoopCacheAnalysis, and the
problem is that there are cases where
isProfitablePerLoopCacheAnalysis(A, B) returns true but
isProfitablePerLoopCacheAnalysis(B, A) returns nullopt. This happens
when the cost computed by CacheCostAnalysis is the same for A and B. In
this case, the result of isProfitablePerLoopCacheAnalysis depends on the
value of CostMap, which maps a loop to an index that is pre-computed in
some way: If CostMap[A] is less than CostMap[B], it returns true,
otherwise it returns nullopt, where the above problem occurs. To avoid
such a situation, if the cost of CacheCostAnalysis is the same for A and
B, it should return nullopt regardless of CostMap.

Author: kasuga-fj

llvm/lib/Transforms/Scalar/LoopInterchange.cpp

@@ -1141,17 +1141,15 @@ LoopInterchangeProfitability::isProfitablePerLoopCacheAnalysis(
   if (OuterLoopIt == CostMap.end())
     return std::nullopt;
 
+  if (CC->getLoopCost(*OuterLoop) == CC->getLoopCost(*InnerLoop))
+    return std::nullopt;
   unsigned InnerIndex = InnerLoopIt->second;
   unsigned OuterIndex = OuterLoopIt->second;
   LLVM_DEBUG(dbgs() << "InnerIndex = " << InnerIndex
                     << ", OuterIndex = " << OuterIndex << "\n");
-  if (InnerIndex < OuterIndex)
-    return std::optional<bool>(true);
   assert(InnerIndex != OuterIndex && "CostMap should assign unique "
                                      "numbers to each loop");
-  if (CC->getLoopCost(*OuterLoop) == CC->getLoopCost(*InnerLoop))
-    return std::nullopt;
-  return std::optional<bool>(false);
+  return std::optional<bool>(InnerIndex < OuterIndex);
 }
 
 std::optional<bool>

llvm/test/Transforms/LoopInterchange/profitability-redundant-interchange.ll

@@ -0,0 +1,81 @@
+; RUN: opt < %s -passes=loop-interchange -cache-line-size=1 -pass-remarks-output=%t -disable-output \
+; RUN:      -verify-dom-info -verify-loop-info
+; RUN: FileCheck -input-file %t %s
+
+
+; Test that loop-interchange doesn't undo its own transoformation. This is
+; the case when the cost computed by CacheCost is the same for the loop of `j`
+; and `k`.
+;
+; #define N 4
+; int a[N*N][N*N][N*N];
+; void f() {
+;   for (int i = 0; i < N; i++)
+;     for (int j = 1; j < 2*N; j++)
+;       for (int k = 1; k < 2*N; k++)
+;         a[i][k+1][j-1] -= a[i+N-1][k][j];
+; }
+
+; CHECK:      --- !Passed
+; CHECK-NEXT: Pass:            loop-interchange
+; CHECK-NEXT: Name:            Interchanged
+; CHECK-NEXT: Function:        f
+; CHECK-NEXT: Args:
+; CHECK-NEXT:    - String:          Loop interchanged with enclosing loop.
+; CHECK-NEXT: ...
+; CHECK-NEXT: --- !Missed
+; CHECK-NEXT: Pass:            loop-interchange
+; CHECK-NEXT: Name:            Dependence
+; CHECK-NEXT: Function:        f
+; CHECK-NEXT: Args:
+; CHECK-NEXT:  - String:       Cannot interchange loops due to dependences.
+; CHECK-NEXT: ...
+; CHECK-NEXT: --- !Missed
+; CHECK-NEXT: Pass:            loop-interchange
+; CHECK-NEXT: Name:            InterchangeNotProfitable
+; CHECK-NEXT: Function:        f
+; CHECK-NEXT: Args:
+; CHECK-NEXT:   - String:          Interchanging loops is not considered to improve cache locality nor vectorization.
+; CHECK-NEXT: ...
+
+@a = dso_local local_unnamed_addr global [16 x [16 x [16 x i32]]] zeroinitializer, align 4
+
+define dso_local void @f() {
+entry:
+  br label %for.cond1.preheader
+
+for.cond1.preheader:
+  %indvars.iv46 = phi i64 [ 0, %entry ], [ %indvars.iv.next47, %for.cond.cleanup3 ]
+  %0 = add nuw nsw i64 %indvars.iv46, 3
+  br label %for.cond5.preheader
+
+for.cond5.preheader:
+  %indvars.iv41 = phi i64 [ 1, %for.cond1.preheader ], [ %indvars.iv.next42, %for.cond.cleanup7 ]
+  %1 = add nsw i64 %indvars.iv41, -1
+  br label %for.body8
+
+for.cond.cleanup3:
+  %indvars.iv.next47 = add nuw nsw i64 %indvars.iv46, 1
+  %exitcond50 = icmp ne i64 %indvars.iv.next47, 4
+  br i1 %exitcond50, label %for.cond1.preheader, label %for.cond.cleanup
+
+for.cond.cleanup7:
+  %indvars.iv.next42 = add nuw nsw i64 %indvars.iv41, 1
+  %exitcond45 = icmp ne i64 %indvars.iv.next42, 8
+  br i1 %exitcond45, label %for.cond5.preheader, label %for.cond.cleanup3
+
+for.body8:
+  %indvars.iv = phi i64 [ 1, %for.cond5.preheader ], [ %indvars.iv.next, %for.body8 ]
+  %arrayidx12 = getelementptr inbounds nuw [16 x [16 x [16 x i32]]], ptr @a, i64 0, i64 %0, i64 %indvars.iv, i64 %indvars.iv41
+  %2 = load i32, ptr %arrayidx12, align 4
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
+  %arrayidx20 = getelementptr inbounds [16 x [16 x [16 x i32]]], ptr @a, i64 0, i64 %indvars.iv46, i64 %indvars.iv.next, i64 %1
+  %3 = load i32, ptr %arrayidx20, align 4
+  %sub21 = sub nsw i32 %3, %2
+  store i32 %sub21, ptr %arrayidx20, align 4
+  %exitcond = icmp ne i64 %indvars.iv.next, 8
+  br i1 %exitcond, label %for.body8, label %for.cond.cleanup7
+
+for.cond.cleanup:
+  ret void
+}