[NFC][Utils] Clone basic blocks after we're done with metadata in CloneFunctionInto

[artempyanykh]

Stacked PRs:

• [Coro] Use DebugInfoCache to speed up cloning in CoroSplitPass #118630
• [Analysis] Add DebugInfoCache analysis #118629
• [Coro] Prebuild a module-level debug info set and share it between all coroutine clones #118628
• [Utils] Identity map module-level debug info on first use in CloneFunction* #118627
• [NFC][Utils] Eliminate DISubprogram set from BuildDebugInfoMDMap #118625
• [NFC][Utils] Extract CloneFunctionBodyInto from CloneFunctionInto #118624
• [NFC][Utils] Extract CloneFunctionMetadataInto from CloneFunctionInto #118623
• [NFC][Utils] Extract BuildDebugInfoMDMap from CloneFunctionInto #118622
• ->[NFC][Utils] Clone basic blocks after we're done with metadata in CloneFunctionInto #118621

---

[NFC][Utils] Clone basic blocks after we're done with metadata in CloneFunctionInto

Summary:
Moving the cloning of BBs after the metadata makes the flow of the
function a bit more straightforward and makes it easier to extract more
into helper functions.

Test Plan:
ninja check-llvm-unit check-llvm

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Artem Pianykh (artempyanykh)

Changes

[NFC][Utils] Clone basic blocks after we're done with metadata in CloneFunctionInto

Summary:
Moving the cloning of BBs after the metadata makes the flow of the
function a bit more straightforward and makes it easier to extract more
into helper functions.

Test Plan:
ninja check-llvm-unit check-llvm

---

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

1 Files Affected:

• (modified) llvm/lib/Transforms/Utils/CloneFunction.cpp (+28-28)

diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp
index b55776d736e663..d038117090e4cc 100644
--- a/llvm/lib/Transforms/Utils/CloneFunction.cpp
+++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp
@@ -210,34 +210,6 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc,
   DISubprogram *SPClonedWithinModule =
       CollectDebugInfoForCloning(*OldFunc, Changes, DIFinder);
 
-  // Loop over all of the basic blocks in the function, cloning them as
-  // appropriate.  Note that we save BE this way in order to handle cloning of
-  // recursive functions into themselves.
-  for (const BasicBlock &BB : *OldFunc) {
-
-    // Create a new basic block and copy instructions into it!
-    BasicBlock *CBB = CloneBasicBlock(&BB, VMap, NameSuffix, NewFunc, CodeInfo);
-
-    // Add basic block mapping.
-    VMap[&BB] = CBB;
-
-    // It is only legal to clone a function if a block address within that
-    // function is never referenced outside of the function.  Given that, we
-    // want to map block addresses from the old function to block addresses in
-    // the clone. (This is different from the generic ValueMapper
-    // implementation, which generates an invalid blockaddress when
-    // cloning a function.)
-    if (BB.hasAddressTaken()) {
-      Constant *OldBBAddr = BlockAddress::get(const_cast<Function *>(OldFunc),
-                                              const_cast<BasicBlock *>(&BB));
-      VMap[OldBBAddr] = BlockAddress::get(NewFunc, CBB);
-    }
-
-    // Note return instructions for the caller.
-    if (ReturnInst *RI = dyn_cast<ReturnInst>(CBB->getTerminator()))
-      Returns.push_back(RI);
-  }
-
   if (Changes < CloneFunctionChangeType::DifferentModule &&
       DIFinder.subprogram_count() > 0) {
     // Turn on module-level changes, since we need to clone (some of) the
@@ -289,6 +261,34 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc,
                                                 TypeMapper, Materializer));
   }
 
+  // Loop over all of the basic blocks in the function, cloning them as
+  // appropriate.  Note that we save BE this way in order to handle cloning of
+  // recursive functions into themselves.
+  for (const BasicBlock &BB : *OldFunc) {
+
+    // Create a new basic block and copy instructions into it!
+    BasicBlock *CBB = CloneBasicBlock(&BB, VMap, NameSuffix, NewFunc, CodeInfo);
+
+    // Add basic block mapping.
+    VMap[&BB] = CBB;
+
+    // It is only legal to clone a function if a block address within that
+    // function is never referenced outside of the function.  Given that, we
+    // want to map block addresses from the old function to block addresses in
+    // the clone. (This is different from the generic ValueMapper
+    // implementation, which generates an invalid blockaddress when
+    // cloning a function.)
+    if (BB.hasAddressTaken()) {
+      Constant *OldBBAddr = BlockAddress::get(const_cast<Function *>(OldFunc),
+                                              const_cast<BasicBlock *>(&BB));
+      VMap[OldBBAddr] = BlockAddress::get(NewFunc, CBB);
+    }
+
+    // Note return instructions for the caller.
+    if (ReturnInst *RI = dyn_cast<ReturnInst>(CBB->getTerminator()))
+      Returns.push_back(RI);
+  }
+
   // Loop over all of the instructions in the new function, fixing up operand
   // references as we go. This uses VMap to do all the hard work.
   for (Function::iterator

[TylerNowicki]

Looks like you are just moving around some code that doesn't have any ordering dependency. LGTM.

[artempyanykh]

@TylerNowicki that's right – the bottom half of the stack is mainly massaging the code into a shape that makes it easier to make functional changes.