[PATCH] D33850: Inlining: Don't re-map simplified cloned instructions.

[Kyle Butt via Phabricator via llvm-commits]

iteratee added inline comments.


================
Comment at: lib/Transforms/Utils/CloneFunction.cpp:457
   SmallVector<const PHINode*, 16> PHIToResolve;
-  for (const BasicBlock &BI : *OldFunc) {
+  for (const BasicBlock &BI : make_range(OldFunc->begin(), OldFunc->end())) {
     Value *V = VMap.lookup(&BI);
----------------
chandlerc wrote:
> iteratee wrote:
> > eraman wrote:
> > > Unrelated change?
> > Not unrelated. If you're inlining a function into itself, end() changes. I can make it a separate patch if you'd like, but I put it in this patch on purpose.
> I'd make it a separate patch (or update the patch description to talk about it).
> 
> But I also don't think this is quite correct -- even the new version computes the end iterator once and caches it, so I'm not sure how this fixes the issue cited.
This isn't necessary for correctness, so I'll submit it separately. It removes unnecessary work.

What happens is that the blocks are batch cloned and then inserted. None of the new blocks are in the map, so nothing bad happens, but it's easier to reason about that if you don't visit the new blocks in the first place.


Repository:
  rL LLVM

https://reviews.llvm.org/D33850