[PATCH] D66892: [BasicBlockUtils] Add metadata fixing in SplitBlockPredecessors.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:582
   BranchInst *BI = BranchInst::Create(BB, NewBB);
+  bool IsBBHeader = LI && LI->isLoopHeader(BB);
+  Loop *BBLoop = LI ? LI->getLoopFor(BB) : nullptr;
   // Splitting the predecessors of a loop header creates a preheader block.
-  if (LI && LI->isLoopHeader(BB))
----------------
Note that is is only the case if `Pred` contains __all__ predecessors from outside the loop. If there is an edge to `BB` left, the new block does not match the definition of a preheader. If Pred contains a backedge, the new block will be inside the loop.


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:602
+    if (IsBBHeader && BBLoop->contains(Pred) && BBLoop->isLoopLatch(Pred)) {
+      // Update loop metadata if it exists.
+      if (MDNode *LoopMD = PI->getMetadata(LLVMContext::MD_loop)) {
----------------
Could you be more specific in the comment, such as:
```
// Move the loop metadata from the old latch terminator to the new basic block, which replaces BB as the latch.
```


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:605
+        BI->setMetadata(LLVMContext::MD_loop, LoopMD);
+        PI->setMetadata(LLVMContext::MD_loop, nullptr);
+      }
----------------
`BB` may continue to be a latch if it has multiple edges to the header, typical for switch terminators.

However, I am not sure it is worth adding a test. We could just leave the LoopMD at the old terminator as well since without being a latch, it should never be used.

(Could you add a test for this case, independent of what we decide to do?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66892/new/

https://reviews.llvm.org/D66892