[LoopUnroll] Improve debug locations for instructions created during loop unrolling.

[samsonov]

Summary

Commit 1

[LoopUnroll] Use IRBuilder to create branch instructions.

Use IRBuilder::Create(Cond)?Br instead of constructing instructions
manually with BranchInst::Create(). It's consistent with other
uses of IRBuilder in this pass, and has an additional important
benefit:

Using IRBuilder will ensure that new branch instruction will get
the same debug location as original terminator instruction it will
eventually replace.

For now I'm not adding a testcase, as currently original terminator
instruction also lack debug location due to missing debug location
propagation in BasicBlock::splitBasicBlock. That is, the testcase
will accompany the fix for the latter I'm going to mail soon.

Commit 2

Set proper debug location for branch added in BasicBlock::splitBasicBlock().

This improves debug locations in passes that do a lot of basic block
transformations. Important case is LoopUnroll pass, the test for correct
debug locations accompanies this change.

[dblaikie]

Looks reasonable (though your description of two separate commits was a bit confusing, since the code review doesn't break it out/make it clear which parts are which)

[samsonov]

Thanks for the review! Sorry about unclear commit breakdown, I failed to use arc correctly this time =/

lib/IR/BasicBlock.cpp

BasicBlock *BasicBlock::splitBasicBlock(iterator I, const Twine &BBName) {
   assert(I != InstList.end() &&
          "Trying to get me to create degenerate basic block!");
 
   BasicBlock *InsertBefore = std::next(Function::iterator(this))
                                .getNodePtrUnchecked();
   BasicBlock *New = BasicBlock::Create(getContext(), BBName,
                                        getParent(), InsertBefore);
 
+  // Save DebugLoc of split point before invalidating iterator.
+  DebugLoc Loc = I->getDebugLoc();
   // Move all of the specified instructions from the original basic block into
   // the new basic block.
   New->getInstList().splice(New->end(), this->getInstList(), I, end());
 
   // Add a branch instruction to the newly formed basic block.
-  BranchInst::Create(New, this);
+  BranchInst *BI = BranchInst::Create(New, this);
+  BI->setDebugLoc(Loc);
 
   // Now we must loop through all of the successors of the New block (which
   // _were_ the successors of the 'this' block), and update any PHI nodes in
   // successors.  If there were PHI nodes in the successors, then they need to
   // know that incoming branches will be from New, not from Old.
   //
   for (succ_iterator I = succ_begin(New), E = succ_end(New); I != E; ++I) {
     // Loop over any phi nodes in the basic block, updating the BB field of

lib/Transforms/Utils/LoopUnrollRuntime.cpp

for (BasicBlock::iterator BBI = (*SBI)->begin();
         PN->addIncoming(NewPN, PrologEnd);
       }
     }
   }
 
   // Create a branch around the orignal loop, which is taken if there are no
   // iterations remaining to be executed after running the prologue.
   Instruction *InsertPt = PrologEnd->getTerminator();
+  IRBuilder<> B(InsertPt);
 
   assert(Count != 0 && "nonsensical Count!");
 
   // If BECount <u (Count - 1) then (BECount + 1) & (Count - 1) == (BECount + 1)
   // (since Count is a power of 2).  This means %xtraiter is (BECount + 1) and
   // and all of the iterations of this loop were executed by the prologue.  Note
   // that if BECount <u (Count - 1) then (BECount + 1) cannot unsigned-overflow.
-  Instruction *BrLoopExit =
-    new ICmpInst(InsertPt, ICmpInst::ICMP_ULT, BECount,
-                 ConstantInt::get(BECount->getType(), Count - 1));
+  Value *BrLoopExit =
+      B.CreateICmpULT(BECount, ConstantInt::get(BECount->getType(), Count - 1));
   BasicBlock *Exit = L->getUniqueExitBlock();
   assert(Exit && "Loop must have a single exit block only");
   // Split the exit to maintain loop canonicalization guarantees
   SmallVector<BasicBlock*, 4> Preds(pred_begin(Exit), pred_end(Exit));
   SplitBlockPredecessors(Exit, Preds, ".unr-lcssa", AA, DT, LI,
                          P->mustPreserveAnalysisID(LCSSAID));
   // Add the branch to the exit block (around the unrolled loop)
-  BranchInst::Create(Exit, NewPH, BrLoopExit, InsertPt);
+  B.CreateCondBr(BrLoopExit, Exit, NewPH);
   InsertPt->eraseFromParent();
 }
 
 /// Create a clone of the blocks in a loop and connect them together.
 /// If UnrollProlog is true, loop structure will not be cloned, otherwise a new
 /// loop will be created including all cloned blocks, and the iterator of it
 /// switches to count NewIter down to 0.
 ///
for (LoopBlocksDFS::RPOIterator BB = BlockBegin; BB != BlockEnd; ++BB) {
 
     }
     if (Latch == *BB) {
       // For the last block, if UnrollProlog is true, create a direct jump to
       // InsertBot. If not, create a loop back to cloned head.
       VMap.erase((*BB)->getTerminator());
       BasicBlock *FirstLoopBB = cast<BasicBlock>(VMap[Header]);
       BranchInst *LatchBR = cast<BranchInst>(NewBB->getTerminator());
+      IRBuilder<> Builder(LatchBR);
       if (UnrollProlog) {
-        LatchBR->eraseFromParent();
-        BranchInst::Create(InsertBot, NewBB);
+        Builder.CreateBr(InsertBot);
       } else {
         PHINode *NewIdx = PHINode::Create(NewIter->getType(), 2, "prol.iter",
                                           FirstLoopBB->getFirstNonPHI());
-        IRBuilder<> Builder(LatchBR);
         Value *IdxSub =
             Builder.CreateSub(NewIdx, ConstantInt::get(NewIdx->getType(), 1),
                               NewIdx->getName() + ".sub");
         Value *IdxCmp =
             Builder.CreateIsNotNull(IdxSub, NewIdx->getName() + ".cmp");
-        BranchInst::Create(FirstLoopBB, InsertBot, IdxCmp, NewBB);
+        Builder.CreateCondBr(IdxCmp, FirstLoopBB, InsertBot);
         NewIdx->addIncoming(NewIter, InsertTop);
         NewIdx->addIncoming(IdxSub, NewBB);
-        LatchBR->eraseFromParent();
       }
+      LatchBR->eraseFromParent();
     }
   }
 
   // Change the incoming values to the ones defined in the preheader or
   // cloned loop.
   for (BasicBlock::iterator I = Header->begin(); isa<PHINode>(I); ++I) {
     PHINode *NewPHI = cast<PHINode>(VMap[I]);
     if (UnrollProlog) {
bool llvm::UnrollRuntimeLoopProlog(Loop *L, unsigned Count,
   // number of iterations that remain to be run in the original loop is a
   // multiple Count == (1 << Log2(Count)) because Log2(Count) <= BEWidth (we
   // explicitly check this above).
 
   Value *BranchVal = B.CreateIsNotNull(ModVal, "lcmp.mod");
 
   // Branch to either the extra iterations or the cloned/unrolled loop
   // We will fix up the true branch label when adding loop body copies
-  BranchInst::Create(PEnd, PEnd, BranchVal, PreHeaderBR);
+  B.CreateCondBr(BranchVal, PEnd, PEnd);
   assert(PreHeaderBR->isUnconditional() &&
          PreHeaderBR->getSuccessor(0) == PEnd &&
          "CFG edges in Preheader are not correct");
   PreHeaderBR->eraseFromParent();
   Function *F = Header->getParent();
   // Get an ordered list of blocks in the loop to help with the ordering of the
   // cloned blocks in the prolog code
   LoopBlocksDFS LoopBlocks(L);

test/Transforms/LoopUnroll/runtime-loop1.ll

 ; RUN: opt < %s -S -loop-unroll -unroll-runtime -unroll-count=2 | FileCheck %s
 
 ; This tests that setting the unroll count works
 
+; CHECK: for.body.preheader:
+; CHECK:   br {{.*}} label %for.body.prol, label %for.body.preheader.split, !dbg [[PH_LOC:![0-9]+]]
 ; CHECK: for.body.prol:
-; CHECK: br label %for.body.preheader.split
+; CHECK:   br label %for.body.preheader.split, !dbg [[BODY_LOC:![0-9]+]]
+; CHECK: for.body.preheader.split:
+; CHECK:   br {{.*}} label %for.end.loopexit, label %for.body.preheader.split.split, !dbg [[PH_LOC]]
 ; CHECK: for.body:
-; CHECK: br i1 %exitcond.1, label %for.end.loopexit.unr-lcssa, label %for.body
+; CHECK:   br i1 %exitcond.1, label %for.end.loopexit.unr-lcssa, label %for.body, !dbg [[BODY_LOC]]
 ; CHECK-NOT: br i1 %exitcond.4, label %for.end.loopexit{{.*}}, label %for.body
 
+; CHECK-DAG: [[PH_LOC]] = !DILocation(line: 101, column: 1, scope: !{{.*}})
+; CHECK-DAG: [[BODY_LOC]] = !DILocation(line: 102, column: 1, scope: !{{.*}})
+
 define i32 @test(i32* nocapture %a, i32 %n) nounwind uwtable readonly {
 entry:
-  %cmp1 = icmp eq i32 %n, 0
-  br i1 %cmp1, label %for.end, label %for.body
+  %cmp1 = icmp eq i32 %n, 0, !dbg !7
+  br i1 %cmp1, label %for.end, label %for.body, !dbg !7
 
 for.body:                                         ; preds = %for.body, %entry
   %indvars.iv = phi i64 [ %indvars.iv.next, %for.body ], [ 0, %entry ]
   %sum.02 = phi i32 [ %add, %for.body ], [ 0, %entry ]
-  %arrayidx = getelementptr inbounds i32, i32* %a, i64 %indvars.iv
-  %0 = load i32, i32* %arrayidx, align 4
-  %add = add nsw i32 %0, %sum.02
-  %indvars.iv.next = add i64 %indvars.iv, 1
-  %lftr.wideiv = trunc i64 %indvars.iv.next to i32
-  %exitcond = icmp eq i32 %lftr.wideiv, %n
-  br i1 %exitcond, label %for.end, label %for.body
+  %arrayidx = getelementptr inbounds i32, i32* %a, i64 %indvars.iv, !dbg !8
+  %0 = load i32, i32* %arrayidx, align 4, !dbg !8
+  %add = add nsw i32 %0, %sum.02, !dbg !8
+  %indvars.iv.next = add i64 %indvars.iv, 1, !dbg !9
+  %lftr.wideiv = trunc i64 %indvars.iv.next to i32, !dbg !9
+  %exitcond = icmp eq i32 %lftr.wideiv, %n, !dbg !9
+  br i1 %exitcond, label %for.end, label %for.body, !dbg !9
 
 for.end:                                          ; preds = %for.body, %entry
   %sum.0.lcssa = phi i32 [ 0, %entry ], [ %add, %for.body ]
-  ret i32 %sum.0.lcssa
+  ret i32 %sum.0.lcssa, !dbg !10
 }
+
+!llvm.module.flags = !{!0, !1, !2}
+!0 = !{i32 2, !"Dwarf Version", i32 4}
+!1 = !{i32 2, !"Debug Info Version", i32 3}
+!2 = !{i32 1, !"PIC Level", i32 2}
+
+!3 = !{}
+!4 = !DISubroutineType(types: !3)
+!5 = !DIFile(filename: "test.cpp", directory: "/tmp")
+!6 = !DISubprogram(name: "test", scope: !5, file: !5, line: 99, type: !4, isLocal: false, isDefinition: true, scopeLine: 100, flags: DIFlagPrototyped, isOptimized: false, function: i32 (i32*, i32)* @test, variables: !3)
+!7 = !DILocation(line: 100, column: 1, scope: !6)
+!8 = !DILocation(line: 101, column: 1, scope: !6)
+!9 = !DILocation(line: 102, column: 1, scope: !6)
+!10 = !DILocation(line: 103, column: 1, scope: !6)