[LoopNest] False negative of `arePerfectlyNested` with LCSSA loops

[TaWeiTu]

Summary

The LCSSA pass (required for all loop passes) sometimes adds additional blocks containing LCSSA variables, and checkLoopsStructure may return false even when the loops are perfectly nested in this case.
This is because the successor of the exit block of the inner loop now points to the LCSSA block instead of the latch block of the outer loop.
Examples are shown in the test nests-with-lcssa.ll.

To fix the issue, the successor of the exit block of the inner loop can now point to a block in which all instructions are LCSSA phi node (except the terminator), and the sole successor of that block should point to the latch block of the outer loop.

[Whitney]

arePerfectlyNested is written very conservative to start with, this patch allows loop nests with PHINodes in between to be considered perfect.

llvm/lib/Analysis/LoopNestAnalysis.cpp

232

How about taking a reference, so no need to check for nullptr.

239

Don't think checking name is ideal, because it can be changed in between passes.
How about PHINodes that have single incoming entry?

272

Use const BasicBlock *

llvm/test/Analysis/LoopNestAnalysis/nests-with-lcssa.ll

1

can the LLVMIR in this LIT be already after loop-rotate, so it doesn't need to reply on loop rotate.

11

Function Attrs are not needed, so can all be removed.

13

please rename the loop header name after rotation, so they can be easily identify.
e.g. for.i for the loop header of loop i.

[TaWeiTu]

Fix LCSSA block checking. Rename loops in the test file.

[TaWeiTu]

llvm/lib/Analysis/LoopNestAnalysis.cpp

239

The LCSSA PHINodes may have two incoming entries, one from the exit block of the inner loop and the other from the header of the outer loop.
I'm not entirely sure about whether the condition is correct and sufficient, though.

[Whitney]

llvm/lib/Analysis/LoopNestAnalysis.cpp

239

The LCSSA PHINodes may have two incoming entries, one from the exit block of the inner loop and the other from the header of the outer loop.

I guess you mean from the exiting block of the inner loop? I thought LCSSA blocks are the exit blocks.
https://llvm.org/docs/LoopTerminology.html#loop-closed-ssa-lcssa
they can just iterate over all the (loop closing) PHI nodes in the exit blocks

I see what you mean by looking at the LIT test, in function f, %split is actually the LCSSA phi, not %res.1.lcssa. %res.1.lcssa is just a generic PHINode.

[TaWeiTu]

llvm/lib/Analysis/LoopNestAnalysis.cpp

239

Oh I see. Thanks for clarifying!
Should I update the name of IsLCSSABlock to something else?

[Whitney]

llvm/lib/Analysis/LoopNestAnalysis.cpp

239

Right, need to be renamed as it is not a LCSSA block (i.e. loop exit block).
Thinking of what we are trying to allow for perfect nest in this patch....
one extra block (optional) between inner exit block and outer latch block, which only contains PHINodes.

That extra basic block is needed because the inner loop is guarded and there exists LCSSA PHINodes in the inner loop exit block.

Can we add this logic in?

[TaWeiTu]

Check the existence of the extra block only when the inner loop is guarded and the inner loop exit contains LCSSA Phi instructions.

[Whitney]

llvm/lib/Analysis/LoopNestAnalysis.cpp

232

Rename B to ExitBlock.

[fhahn]

llvm/lib/Analysis/LoopNestAnalysis.cpp

233

You only need to check phi nodes. There's B.phis(). Also, usually it is more common to use BB for BasicBlock variables.

239

It might be helpful to add a comment here to specify what 'extra phi block' means.

Also, If you just need to look at PHI nodes, I think you can use B.phis().

[TaWeiTu]

Rename variables. Use BasicBlock::phis(). Explain what "extra Phi block" means in the comment.

[TaWeiTu]

Hi @Whitney, @fhahn, thanks for your review and comment!
Please let me know if there's any other problem with the patch.
Also, if the patch is ready to land, is it possible for you to commit it instead (I believe I don't have the commit access)?

[Whitney]

In D86133#2226251, @TaWeiTu wrote:

Hi @Whitney, @fhahn, thanks for your review and comment!
Please let me know if there's any other problem with the patch.
Also, if the patch is ready to land, is it possible for you to commit it instead (I believe I don't have the commit access)?

One minor comment, then LGTM. Let's see what Florian thinks.

Instruction to obtain commit access:
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Prior to obtaining commit access, it is common practice to request that someone with commit access commits on your behalf. When doing so, please provide the name and email address you would like to use in the Author property of the commit.

--author="Ta-Wei Tu <[e-mail]>" Good?

llvm/lib/Analysis/LoopNestAnalysis.cpp

245

Usually blocks of PHINodes are named IncomingBlock

[TaWeiTu]

Source -> IncomingBlock

[TaWeiTu]

In D86133#2227414, @Whitney wrote:

--author="Ta-Wei Tu <[e-mail]>" Good?

Sure, thank you very much!

[fhahn]

In D86133#2227414, @Whitney wrote:

In D86133#2226251, @TaWeiTu wrote:

Hi @Whitney, @fhahn, thanks for your review and comment!
Please let me know if there's any other problem with the patch.
Also, if the patch is ready to land, is it possible for you to commit it instead (I believe I don't have the commit access)?

One minor comment, then LGTM. Let's see what Florian thinks.

Looks like my comments have been addressed thanks!

llvm/lib/Analysis/LoopNestAnalysis.cpp

233

This only uses PN in the closure, right? So no need to use [&].

[TaWeiTu]

Replace unnecessary [&] with [].

[TaWeiTu]

Hi @etiotto, @Whitney thanks for your reviews!
Can you commit this for me? --author="Ta-Wei Tu <[e-mail]>" as mentioned above looks good to me, thanks!

[Whitney]

In D86133#2236450, @TaWeiTu wrote:

Hi @etiotto, @Whitney thanks for your reviews!
Can you commit this for me? --author="Ta-Wei Tu <[e-mail]>" as mentioned above looks good to me, thanks!

Sure. I am swamped with other work, finally have time to get to it today.

[TaWeiTu]

In D86133#2236462, @Whitney wrote:

In D86133#2236450, @TaWeiTu wrote:

Hi @etiotto, @Whitney thanks for your reviews!
Can you commit this for me? --author="Ta-Wei Tu <[e-mail]>" as mentioned above looks good to me, thanks!

Sure. I am swamped with other work, finally have time to get to it today.

Thanks!

llvm/lib/Analysis/LoopNestAnalysis.cpp

static bool checkLoopsStructure(const Loop &OuterLoop, const Loop &InnerLoop,
   const BasicBlock *InnerLoopLatch = InnerLoop.getLoopLatch();
   const BasicBlock *InnerLoopExit = InnerLoop.getExitBlock();
 
   // We expect rotated loops. The inner loop should have a single exit block.
   if (OuterLoop.getExitingBlock() != OuterLoopLatch ||
       InnerLoop.getExitingBlock() != InnerLoopLatch || !InnerLoopExit)
     return false;
 
+  auto isLCSSABlock = [](const BasicBlock *B) {
+    if (!B)
+      return false;
+
+    return all_of(*B, [](const Instruction &I) {
+      if (dyn_cast<BranchInst>(&I))
+        return true;
+      auto *PN = dyn_cast<PHINode>(&I);
+      return PN && PN->getName().endswith(".lcssa");
+    });
+  };
+
   // Ensure the only branch that may exist between the loops is the inner loop
   // guard.
   if (OuterLoopHeader != InnerLoopPreHeader) {
     const BranchInst *BI =
         dyn_cast<BranchInst>(OuterLoopHeader->getTerminator());
 
     if (!BI || BI != InnerLoop.getLoopGuardBranch())
       return false;
 
     // The successors of the inner loop guard should be the inner loop
     // preheader and the outer loop latch.
     for (const BasicBlock *Succ : BI->successors()) {
       if (Succ == InnerLoopPreHeader)
         continue;
       if (Succ == OuterLoopLatch)
         continue;
+      if (isLCSSABlock(Succ) && Succ->getSingleSuccessor() == OuterLoopLatch)
+        continue;
 
       DEBUG_WITH_TYPE(VerboseDebug, {
         dbgs() << "Inner loop guard successor " << Succ->getName()
                << " doesn't lead to inner loop preheader or "
                   "outer loop latch.\n";
       });
       return false;
     }
   }
 
   // Ensure the inner loop exit block leads to the outer loop latch.
-  if (InnerLoopExit->getSingleSuccessor() != OuterLoopLatch) {
+  auto *SuccInner = InnerLoopExit->getSingleSuccessor();
+  if (!SuccInner || (SuccInner != OuterLoopLatch &&
+                     (!isLCSSABlock(SuccInner) ||
+                      SuccInner->getSingleSuccessor() != OuterLoopLatch))) {
     DEBUG_WITH_TYPE(
         VerboseDebug,
         dbgs() << "Inner loop exit block " << *InnerLoopExit
                << " does not directly lead to the outer loop latch.\n";);
     return false;
   }
 
   return true;

llvm/test/Analysis/LoopNestAnalysis/nests-with-lcssa.ll

+; RUN: opt -S -passes='loop-rotate,print<loopnest>' < %s 2>&1 > /dev/null | FileCheck %s
+
+; int f(int N, int M) {
+;   int res = 0;
+;   for (int i = 0; i < N; ++i) {
+;     for (int j = 0; j < M; ++j) res += i * j;
+;   }
+;   return res;
+; }
+
+; Function Attrs: noinline nounwind optnone
+define i32 @f(i32 %N, i32 %M) #0 {
+; CHECK: IsPerfect=true, Depth=1, OutermostLoop: for.body3, Loops: ( for.body3 )
+; CHECK: IsPerfect=true, Depth=2, OutermostLoop: for.body, Loops: ( for.body for.body3 )
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.inc4, %entry
+  %res.0 = phi i32 [ 0, %entry ], [ %res.1, %for.inc4 ]
+  %i.0 = phi i32 [ 0, %entry ], [ %inc5, %for.inc4 ]
+  %cmp = icmp slt i32 %i.0, %N
+  br i1 %cmp, label %for.body, label %for.end6
+
+for.body:                                         ; preds = %for.cond
+  br label %for.cond1
+
+for.cond1:                                        ; preds = %for.inc, %for.body
+  %res.1 = phi i32 [ %res.0, %for.body ], [ %add, %for.inc ]
+  %j.0 = phi i32 [ 0, %for.body ], [ %inc, %for.inc ]
+  %cmp2 = icmp slt i32 %j.0, %M
+  br i1 %cmp2, label %for.body3, label %for.end
+
+for.body3:                                        ; preds = %for.cond1
+  %mul = mul nsw i32 %i.0, %j.0
+  %add = add nsw i32 %res.1, %mul
+  br label %for.inc
+
+for.inc:                                          ; preds = %for.body3
+  %inc = add nsw i32 %j.0, 1
+  br label %for.cond1
+
+for.end:                                          ; preds = %for.cond1
+  br label %for.inc4
+
+for.inc4:                                         ; preds = %for.end
+  %inc5 = add nsw i32 %i.0, 1
+  br label %for.cond
+
+for.end6:                                         ; preds = %for.cond
+  ret i32 %res.0
+}
+
+; int g(int N, int M, int K) {
+;   int sum = 0, prod = 1;
+;   for (int i = 0; i < N; ++i) {
+;     for (int j = 0; j < M; ++j) {
+;       for (int k = 0; k < K; ++k) {
+;         sum += i * j * k;
+;       }
+;       prod *= (i + j);
+;     }
+;   }
+;   return sum + prod;
+; }
+
+; Function Attrs: noinline nounwind optnone
+define i32 @g(i32 %N, i32 %M, i32 %K) #0 {
+; CHECK: IsPerfect=true, Depth=1, OutermostLoop: for.body6, Loops: ( for.body6 )
+; CHECK: IsPerfect=false, Depth=2, OutermostLoop: for.body3, Loops: ( for.body3 for.body6 )
+; CHECK: IsPerfect=false, Depth=3, OutermostLoop: for.body, Loops: ( for.body for.body3 for.body6 )
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.inc13, %entry
+  %prod.0 = phi i32 [ 1, %entry ], [ %prod.1, %for.inc13 ]
+  %sum.0 = phi i32 [ 0, %entry ], [ %sum.1, %for.inc13 ]
+  %i.0 = phi i32 [ 0, %entry ], [ %inc14, %for.inc13 ]
+  %cmp = icmp slt i32 %i.0, %N
+  br i1 %cmp, label %for.body, label %for.end15
+
+for.body:                                         ; preds = %for.cond
+  br label %for.cond1
+
+for.cond1:                                        ; preds = %for.inc10, %for.body
+  %prod.1 = phi i32 [ %prod.0, %for.body ], [ %mul9, %for.inc10 ]
+  %sum.1 = phi i32 [ %sum.0, %for.body ], [ %sum.2, %for.inc10 ]
+  %j.0 = phi i32 [ 0, %for.body ], [ %inc11, %for.inc10 ]
+  %cmp2 = icmp slt i32 %j.0, %M
+  br i1 %cmp2, label %for.body3, label %for.end12
+
+for.body3:                                        ; preds = %for.cond1
+  br label %for.cond4
+
+for.cond4:                                        ; preds = %for.inc, %for.body3
+  %sum.2 = phi i32 [ %sum.1, %for.body3 ], [ %add, %for.inc ]
+  %k.0 = phi i32 [ 0, %for.body3 ], [ %inc, %for.inc ]
+  %cmp5 = icmp slt i32 %k.0, %K
+  br i1 %cmp5, label %for.body6, label %for.end
+
+for.body6:                                        ; preds = %for.cond4
+  %mul = mul nsw i32 %i.0, %j.0
+  %mul7 = mul nsw i32 %mul, %k.0
+  %add = add nsw i32 %sum.2, %mul7
+  br label %for.inc
+
+for.inc:                                          ; preds = %for.body6
+  %inc = add nsw i32 %k.0, 1
+  br label %for.cond4
+
+for.end:                                          ; preds = %for.cond4
+  %add8 = add nsw i32 %i.0, %j.0
+  %mul9 = mul nsw i32 %prod.1, %add8
+  br label %for.inc10
+
+for.inc10:                                        ; preds = %for.end
+  %inc11 = add nsw i32 %j.0, 1
+  br label %for.cond1
+
+for.end12:                                        ; preds = %for.cond1
+  br label %for.inc13
+
+for.inc13:                                        ; preds = %for.end12
+  %inc14 = add nsw i32 %i.0, 1
+  br label %for.cond
+
+for.end15:                                        ; preds = %for.cond
+  %add16 = add nsw i32 %sum.0, %prod.0
+  ret i32 %add16
+}
+
+; int h(int N, int M, int K) {
+;   int sum = 0;
+;   for (int i = 0; i < N; ++i) {
+;     for (int j = 0; j < M; ++j) {
+;       for (int k = 0; k < K; ++k) {
+;         sum += i * j * k;
+;       }
+;     }
+;   }
+;   return sum;
+; }
+
+; Function Attrs: noinline nounwind optnone
+define i32 @h(i32 %N, i32 %M, i32 %K) #0 {
+; CHECK: IsPerfect=true, Depth=1, OutermostLoop: for.body6, Loops: ( for.body6 )
+; CHECK: IsPerfect=true, Depth=2, OutermostLoop: for.body3, Loops: ( for.body3 for.body6 )
+; CHECK: IsPerfect=true, Depth=3, OutermostLoop: for.body, Loops: ( for.body for.body3 for.body6 )
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.inc11, %entry
+  %sum.0 = phi i32 [ 0, %entry ], [ %sum.1, %for.inc11 ]
+  %i.0 = phi i32 [ 0, %entry ], [ %inc12, %for.inc11 ]
+  %cmp = icmp slt i32 %i.0, %N
+  br i1 %cmp, label %for.body, label %for.end13
+
+for.body:                                         ; preds = %for.cond
+  br label %for.cond1
+
+for.cond1:                                        ; preds = %for.inc8, %for.body
+  %sum.1 = phi i32 [ %sum.0, %for.body ], [ %sum.2, %for.inc8 ]
+  %j.0 = phi i32 [ 0, %for.body ], [ %inc9, %for.inc8 ]
+  %cmp2 = icmp slt i32 %j.0, %M
+  br i1 %cmp2, label %for.body3, label %for.end10
+
+for.body3:                                        ; preds = %for.cond1
+  br label %for.cond4
+
+for.cond4:                                        ; preds = %for.inc, %for.body3
+  %sum.2 = phi i32 [ %sum.1, %for.body3 ], [ %add, %for.inc ]
+  %k.0 = phi i32 [ 0, %for.body3 ], [ %inc, %for.inc ]
+  %cmp5 = icmp slt i32 %k.0, %K
+  br i1 %cmp5, label %for.body6, label %for.end
+
+for.body6:                                        ; preds = %for.cond4
+  %mul = mul nsw i32 %i.0, %j.0
+  %mul7 = mul nsw i32 %mul, %k.0
+  %add = add nsw i32 %sum.2, %mul7
+  br label %for.inc
+
+for.inc:                                          ; preds = %for.body6
+  %inc = add nsw i32 %k.0, 1
+  br label %for.cond4
+
+for.end:                                          ; preds = %for.cond4
+  br label %for.inc8
+
+for.inc8:                                         ; preds = %for.end
+  %inc9 = add nsw i32 %j.0, 1
+  br label %for.cond1
+
+for.end10:                                        ; preds = %for.cond1
+  br label %for.inc11
+
+for.inc11:                                        ; preds = %for.end10
+  %inc12 = add nsw i32 %i.0, 1
+  br label %for.cond
+
+for.end13:                                        ; preds = %for.cond
+  ret i32 %sum.0
+}