Keep the order of the basic blocks in the cloned loop as the original loop

[Whitney]

Summary

Do the cloning in two steps, first allocate all the new loops, then clone the basic blocks in the same order as the original loop.

[hfinkel]

Do you have any kind of test case? I suppose that you can check the output in a test in test/Analysis/LoopInfo to show the ordering?

llvm/lib/Transforms/Utils/CloneFunction.cpp

783

Is this logic equivalent? The original code is going a pre-order traversal, and for each loop, it excludes blocks in an inner loop (when CurLoop != LI->getLoopFor(BB)). Here we're doing something for all blocks in the loop including those also in inner loops?

[Whitney]

llvm/lib/Transforms/Utils/CloneFunction.cpp

783

The resulting cloned basic blocks will be the same, which are all the blocks in OrigLoop. The different is the order of the basic block. Example:
Given OrigLoop:

OuterHeader:
  br InnerHeader
InnerHeader:
  br InnerHeader or OuterLatch
OuterLatch:
  br OuterHeader or OuterExit

Output before this change:

ClonedOuterHeader:
  br ClonedInnerHeader
ClonedOuterLatch:
  br ClonedOuterHeader or ClonedOuterExit
ClonedInnerHeader:
  br ClonedInnerHeader or ClonedOuterLatch

Output after this change:

ClonedOuterHeader:
  br ClonedInnerHeader
ClonedInnerHeader:
  br ClonedInnerHeader or ClonedOuterLatch
ClonedOuterLatch:
  br ClonedOuterHeader or ClonedOuterExit

FYI - this function is extended by me in https://reviews.llvm.org/rG7c1deeff4a67296654823a871fea5c1a2aef3b8a to support cloning a loop nest instead of only allow the innermost loop. But I didn't think properly about the ordering of the basic block at that time.

[hfinkel]

llvm/lib/Transforms/Utils/CloneFunction.cpp

783

Ah, okay. I see. This loop is not part of the pre-order traversal, it's just cloning all of the blocks in the outer loop in order. That seems fine.

[Whitney]

@hfinkel Thanks for the review! I am not sure how to add a test in test/Analysis/LoopInfo, as cloneLoopWithPreheader() is not called by LoopInfo, so I added a test in unittests/Transforms/Utils/CloningTest.cpp instead.

[hfinkel]

LGTM

[fhahn]

LGTM, thanks. For the commit message, it would be great if you could prefix the area the patch falls in, e.g. [CloneFunction].

[Whitney]

Already committed on July 8 (7d8f30e6b2f27d55d4a14392951e4a61d7598767).

llvm/lib/Transforms/Utils/CloneFunction.cpp

Loop *llvm::cloneLoopWithPreheader(BasicBlock *Before, BasicBlock *LoopDomBB,
   // Update LoopInfo.
   if (ParentLoop)
     ParentLoop->addBasicBlockToLoop(NewPH, *LI);
 
   // Update DominatorTree.
   DT->addNewBlock(NewPH, LoopDomBB);
 
   for (Loop *CurLoop : OrigLoop->getLoopsInPreorder()) {
-    for (BasicBlock *BB : CurLoop->getBlocks()) {
-      if (CurLoop != LI->getLoopFor(BB))
-        continue;
-
-      Loop *&NewLoop = LMap[CurLoop];
-      if (!NewLoop) {
-        NewLoop = LI->AllocateLoop();
 
-        // Establish the parent/child relationship.
-        Loop *OrigParent = CurLoop->getParentLoop();
-        assert(OrigParent && "Could not find the original parent loop");
-        Loop *NewParentLoop = LMap[OrigParent];
-        assert(NewParentLoop && "Could not find the new parent loop");
 
-        NewParentLoop->addChildLoop(NewLoop);
-      }
+  }
+
+  for (BasicBlock *BB : OrigLoop->getBlocks()) {
+    Loop *CurLoop = LI->getLoopFor(BB);
+    Loop *&NewLoop = LMap[CurLoop];
+    assert(NewLoop && "Expecting new loop to be allocated");
 
-      BasicBlock *NewBB = CloneBasicBlock(BB, VMap, NameSuffix, F);
-      VMap[BB] = NewBB;
 
-      // Update LoopInfo.
-      NewLoop->addBasicBlockToLoop(NewBB, *LI);
-      if (BB == CurLoop->getHeader())
-        NewLoop->moveToHeader(NewBB);
 
-      // Add DominatorTree node. After seeing all blocks, update to correct
-      // IDom.
-      DT->addNewBlock(NewBB, NewPH);
 
-      Blocks.push_back(NewBB);
-    }
-  }
 
   for (BasicBlock *BB : OrigLoop->getBlocks()) {
     // Update DominatorTree.
     BasicBlock *IDomBB = DT->getNode(BB)->getIDom()->getBlock();
     DT->changeImmediateDominator(cast<BasicBlock>(VMap[BB]),
                                  cast<BasicBlock>(VMap[IDomBB]));
   }
 

llvm/unittests/Transforms/Utils/CloningTest.cpp

 //===- Cloning.cpp - Unit tests for the Cloner ----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/Analysis/DomTreeUpdater.h"
+#include "llvm/Analysis/LoopInfo.h"
+#include "llvm/AsmParser/Parser.h"
 #include "llvm/IR/Argument.h"
 #include "llvm/IR/Constant.h"
 #include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstIterator.h"
 #include "llvm/IR/Instructions.h"
TEST_F(CloneInstruction, DuplicateInstructionsToSplitBlocksEq2) {
   EXPECT_EQ(MulSplit->getParent(), Split);
   EXPECT_EQ(MulSplit->getNextNode(), Split->getTerminator());
   EXPECT_EQ(Split->getSingleSuccessor(), BB2);
   EXPECT_EQ(BB2->getSingleSuccessor(), Split);
 
   delete F;
 }
 
+static void runWithLoopInfoAndDominatorTree(
+    Module &M, StringRef FuncName,
+    function_ref<void(Function &F, LoopInfo &LI, DominatorTree &DT)> Test) {
+  auto *F = M.getFunction(FuncName);
+  ASSERT_NE(F, nullptr) << "Could not find " << FuncName;
+
+  DominatorTree DT(*F);
+  LoopInfo LI(DT);
+
+  Test(*F, LI, DT);
+}
+
+static std::unique_ptr<Module> parseIR(LLVMContext &C, const char *IR) {
+  SMDiagnostic Err;
+  std::unique_ptr<Module> Mod = parseAssemblyString(IR, Err, C);
+  if (!Mod)
+    Err.print("CloneLoop", errs());
+  return Mod;
+}
+
+TEST(CloneLoop, CloneLoopNest) {
+  // Parse the module.
+  LLVMContext Context;
+
+  std::unique_ptr<Module> M = parseIR(
+    Context,
+    R"(define void @foo(i32* %A, i32 %ub) {
+entry:
+  %guardcmp = icmp slt i32 0, %ub
+  br i1 %guardcmp, label %for.outer.preheader, label %for.end
+for.outer.preheader:
+  br label %for.outer
+for.outer:
+  %j = phi i32 [ 0, %for.outer.preheader ], [ %inc.outer, %for.outer.latch ]
+  br i1 %guardcmp, label %for.inner.preheader, label %for.outer.latch
+for.inner.preheader:
+  br label %for.inner
+for.inner:
+  %i = phi i32 [ 0, %for.inner.preheader ], [ %inc, %for.inner ]
+  %idxprom = sext i32 %i to i64
+  %arrayidx = getelementptr inbounds i32, i32* %A, i64 %idxprom
+  store i32 %i, i32* %arrayidx, align 4
+  %inc = add nsw i32 %i, 1
+  %cmp = icmp slt i32 %inc, %ub
+  br i1 %cmp, label %for.inner, label %for.inner.exit
+for.inner.exit:
+  br label %for.outer.latch
+for.outer.latch:
+  %inc.outer = add nsw i32 %j, 1
+  %cmp.outer = icmp slt i32 %inc.outer, %ub
+  br i1 %cmp.outer, label %for.outer, label %for.outer.exit
+for.outer.exit:
+  br label %for.end
+for.end:
+  ret void
+})"
+    );
+
+  runWithLoopInfoAndDominatorTree(
+      *M, "foo", [&](Function &F, LoopInfo &LI, DominatorTree &DT) {
+        Function::iterator FI = F.begin();
+        // First basic block is entry - skip it.
+        BasicBlock *Preheader = &*(++FI);
+        BasicBlock *Header = &*(++FI);
+        assert(Header->getName() == "for.outer");
+        Loop *L = LI.getLoopFor(Header);
+        EXPECT_NE(L, nullptr);
+        EXPECT_EQ(Header, L->getHeader());
+        EXPECT_EQ(Preheader, L->getLoopPreheader());
+
+        ValueToValueMapTy VMap;
+        SmallVector<BasicBlock *, 4> ClonedLoopBlocks;
+        Loop *NewLoop = cloneLoopWithPreheader(Preheader, Preheader, L, VMap,
+                                               "", &LI, &DT, ClonedLoopBlocks);
+        EXPECT_NE(NewLoop, nullptr);
+        EXPECT_EQ(NewLoop->getSubLoops().size(), 1u);
+        Loop::block_iterator BI = NewLoop->block_begin();
+        EXPECT_TRUE((*BI)->getName().startswith("for.outer"));
+        EXPECT_TRUE((*(++BI))->getName().startswith("for.inner.preheader"));
+        EXPECT_TRUE((*(++BI))->getName().startswith("for.inner"));
+        EXPECT_TRUE((*(++BI))->getName().startswith("for.inner.exit"));
+        EXPECT_TRUE((*(++BI))->getName().startswith("for.outer.latch"));
+      });
+}
+
 class CloneFunc : public ::testing::Test {
 protected:
   void SetUp() override {
     SetupModule();
     CreateOldFunc();
     CreateNewFunc();
     SetupFinder();
   }