[BasicBlockUtils] Add metadata fixing in SplitBlockPredecessors.

[aus_intel]

Summary

In case when BB is header of some loop and predecessor is latch of
this loop, metadata was not attached to newly created basic block.
This led to loss of loop metadata for other passes.

[Orlando]

I don't feel confident enough in this area to give an official LGTM but If I understand it correctly the patch seems reasonable: move loop metadata from old to new latch when a loop header is split.

I have left some inline comments in the test.

Thanks,
Orlando

llvm/test/Transforms/LoopSimplify/loop_metadata.ll

1

Please could you add a short description describing what the test is for (it's not obvious just looking at the test file)?

It's not a requirement but it is helpful for others if this test ever fails.

4

I think this should be a CHECK-NEXT to ensure this branch is part of the for.cond.loopexit BB.

5

I assume this line is to check that the loop metadata is correctly removed from this block.

However, the CHECK will still pass with:

br i1 %cmp1, label %for.body1, label %for.cond.loopexit, !llvm.loop !0

I think you want the following instead, because {{$}} will match the end of the line.

CHECK: br i1 %cmp1, label %for.body1, label %for.cond.loopexit{{$}}

[Meinersbur]

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp

582

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.

602

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.

605

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?)

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp

BasicBlock *llvm::SplitBlockPredecessors(BasicBlock *BB,
   }
 
   // Create new basic block, insert right before the original block.
   BasicBlock *NewBB = BasicBlock::Create(
       BB->getContext(), BB->getName() + Suffix, BB->getParent(), BB);
 
   // The new block unconditionally branches to the old block.
   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))
+  if (IsBBHeader)
     // Using the loop start line number prevents debuggers stepping into the
     // loop body for this instruction.
-    BI->setDebugLoc(LI->getLoopFor(BB)->getStartLoc());
+    BI->setDebugLoc(BBLoop->getStartLoc());
   else
     BI->setDebugLoc(BB->getFirstNonPHIOrDbg()->getDebugLoc());
 
   // Move the edges from Preds to point to NewBB instead of BB.
-  for (unsigned i = 0, e = Preds.size(); i != e; ++i) {
+  for (BasicBlock *Pred : Preds) {
+    Instruction *PI = Pred->getTerminator();
     // This is slightly more strict than necessary; the minimum requirement
     // is that there be no more than one indirectbr branching to BB. And
     // all BlockAddress uses would need to be updated.
-    assert(!isa<IndirectBrInst>(Preds[i]->getTerminator()) &&
+    assert(!isa<IndirectBrInst>(PI) &&
            "Cannot split an edge from an IndirectBrInst");
-    assert(!isa<CallBrInst>(Preds[i]->getTerminator()) &&
-           "Cannot split an edge from a CallBrInst");
-    Preds[i]->getTerminator()->replaceUsesOfWith(BB, NewBB);
+    assert(!isa<CallBrInst>(PI) && "Cannot split an edge from a CallBrInst");
+    if (IsBBHeader && BBLoop->contains(Pred) && BBLoop->isLoopLatch(Pred)) {
+      // Update loop metadata if it exists.
+      if (MDNode *LoopMD = PI->getMetadata(LLVMContext::MD_loop)) {
+        BI->setMetadata(LLVMContext::MD_loop, LoopMD);
+        PI->setMetadata(LLVMContext::MD_loop, nullptr);
+      }
+    }
+    PI->replaceUsesOfWith(BB, NewBB);
   }
 
   // Insert a new PHI node into NewBB for every PHI node in BB and that new PHI
   // node becomes an incoming value for BB's phi node.  However, if the Preds
   // list is empty, we need to insert dummy entries into the PHI nodes in BB to
   // account for the newly created predecessor.
   if (Preds.empty()) {
     // Insert dummy values as the incoming value.

llvm/test/Transforms/LoopSimplify/loop_metadata.ll

+; RUN: opt -S -loop-simplify < %s | FileCheck %s
+
+; CHECK: for.cond.loopexit:
+; CHECK: br label %for.cond, !llvm.loop !0
+; CHECK: br i1 %cmp1, label %for.body1, label %for.cond.loopexit
+
+define void @foo() {
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.cond1, %entry
+  %j = phi i32 [ 0, %entry ], [ %add, %for.cond1 ]
+  %cmp = icmp ult i32 %j, 8
+  br i1 %cmp, label %for.body, label %for.end
+
+for.body:                                         ; preds = %for.cond
+  %dummy1 = add i32 1, 1
+  %add = add nuw nsw i32 %j, 1
+  br label %for.cond1
+
+for.cond1:                                      ; preds = %for.body1, %for.body
+  %i.0 = phi i32 [ 1, %for.body ], [ %inc, %for.body1 ]
+  %cmp1 = icmp ult i32 %i.0, 8
+  br i1 %cmp1, label %for.body1, label %for.cond, !llvm.loop !0
+
+for.body1:                                      ; preds = %for.cond1
+  %dummy2 = add i32 1, 1
+  %inc = add nuw nsw i32 %i.0, 1
+  br label %for.cond1
+
+for.end:                                       ; preds = %for.cond
+  ret void
+}
+
+!0 = distinct !{!0, !1}
+!1 = !{!"llvm.loop.unroll.full"}