[PATCH] D31281: [LoopUnroll] Remap references in peeled iteration

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added a comment.

Thank you for review.



================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:419
+
+    // Rewrite the cloned instruction operands to use the values created when
+    // the clone is created.
----------------
mkuper wrote:
> sepavloff wrote:
> > mkuper wrote:
> > > 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.
> > 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`.
> 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!
Yes, the problem is only malformed dominator tree. It was found in self builds with expensive checks enabled.


================
Comment at: test/Transforms/LoopUnroll/peel-loop.ll:99
+; 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 {
----------------
mkuper wrote:
> 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.
Done, but I had to move the test into separate file, as with options of `peel-loop.ll` the code changed too substantially.


Repository:
  rL LLVM

https://reviews.llvm.org/D31281