[LoopUnroll] Remap references in peeled iteration

[sepavloff]

Summary

If a loop contains several BBs, peeling produces code in which jump
targets point to original loop blocks rather than cloned BBs. This
patch uses the same way as loop unrolling to remap cloned blocks.

[mkuper]

lib/Transforms/Utils/LoopUnrollPeel.cpp

419

Something here is wrong. There's already a call to remapInstructionsInBlocks in line 443 (original) that does exactly this, so everything ought to be updated correctly. If it isn't, then something's going wrong between here and there, and I'm not sure what it is.

[sepavloff]

lib/Transforms/Utils/LoopUnrollPeel.cpp

419

Indeed, you are right. But the remapping is made too late, it must be done prior to dominator tree calculation, which in turn must be done prior to call to SplitBlock.

[sepavloff]

Updated patch

[mkuper]

LGTM, but please fix the test before committing.

lib/Transforms/Utils/LoopUnrollPeel.cpp

419

Ok, this makes more sense. So what happened here isn't that we generated bad branches, it's that we produced a malformed dominator tree?
Anyway, thanks for catching this!

test/Transforms/LoopUnroll/peel-loop.ll

99

Please make the test actually test its output (in particular, that we actually peeled the loop).
I think you can just run the update script on it.

[sepavloff]

Thank you for review.

lib/Transforms/Utils/LoopUnrollPeel.cpp

419

Yes, the problem is only malformed dominator tree. It was found in self builds with expensive checks enabled.

test/Transforms/LoopUnroll/peel-loop.ll

99

Done, but I had to move the test into separate file, as with options of peel-loop.ll the code changed too substantially.

lib/Transforms/Utils/LoopUnrollPeel.cpp

for (unsigned Iter = 0; Iter < PeelCount; ++Iter) {
     // guard here to make sure we don't end up with non-positive weights.
     if (ExitWeight < CurHeaderWeight)
       CurHeaderWeight -= ExitWeight;
     else
       CurHeaderWeight = 1;
 
     cloneLoopBlocks(L, Iter, InsertTop, InsertBot, Exit,
                     NewBlocks, LoopBlocks, VMap, LVMap, DT, LI);
+
+    // Rewrite the cloned instruction operands to use the values created when
+    // the clone is created.
+    for (BasicBlock *BB : NewBlocks) {
+      for (Instruction &I : *BB) {
+        RemapInstruction(&I, VMap,
+                         RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
+      }
+    }
+
     if (DT) {
       // Latches of the cloned loops dominate over the loop exit, so idom of the
       // latter is the first cloned loop body, as original PreHeader dominates
       // the original loop body.
       if (Iter == 0)
         DT->changeImmediateDominator(Exit, cast<BasicBlock>(LVMap[Latch]));
 #ifndef NDEBUG
       if (VerifyDomInfo)

test/Transforms/LoopUnroll/peel-loop.ll

 
 for.cond.for.end_crit_edge:                       ; preds = %for.body
   br label %for.end
 
 for.end:                                          ; preds = %for.cond.for.end_crit_edge, %entry
   %ret = phi i32 [ 0, %entry], [ %inc, %for.cond.for.end_crit_edge ]
   ret i32 %ret
 }
+
+; Check if loop composed of several BB is peeled correctly.
+declare void @funcb()
+define void @funca(i8* readnone %b, i8* readnone %e) local_unnamed_addr {
+entry:
+  %cmp2 = icmp eq i8* %b, %e
+  br i1 %cmp2, label %for.end, label %for.body.preheader
+
+for.body.preheader:
+  br label %for.body
+
+for.body:
+  %Comma.04 = phi i1 [ true, %if.end ], [ false, %for.body.preheader ]
+  %b.addr.03 = phi i8* [ %incdec.ptr, %if.end ], [ %b, %for.body.preheader ]
+  br i1 %Comma.04, label %if.then, label %if.end
+
+if.then:
+  tail call void @funcb()
+  br label %if.end
+
+if.end:
+  %incdec.ptr = getelementptr inbounds i8, i8* %b.addr.03, i64 1
+  %cmp = icmp eq i8* %incdec.ptr, %e
+  br i1 %cmp, label %for.end.loopexit, label %for.body
+
+for.end.loopexit:
+  br label %for.end
+
+for.end:
+  ret void
+}