[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:
> > 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.
> I'll note that as currently written, this still walks from begin to end of OldFunc, which I'm pretty sure is what the old code did as well. Maybe I'm missing something, but this looks like a no-op change...
I'll pull it out separately, but explain it here.


================
Comment at: lib/Transforms/Utils/CloneFunction.cpp:458
+  for (const BasicBlock &BI : make_range(OldFunc->begin(), OldFunc->end())) {
     Value *V = VMap.lookup(&BI);
     BasicBlock *NewBB = cast_or_null<BasicBlock>(V);
----------------
This line makes it a No-Op when we encounter the blocks added below.

The goal of the change was to make it so you don't have to find this line and do the reasoning yourself.


================
Comment at: lib/Transforms/Utils/CloneFunction.cpp:463
     // Add the new block to the new function.
     NewFunc->getBasicBlockList().push_back(NewBB);
 
----------------
You're missing this line here. When OldFunc == NewFunc, this line changes End.


Repository:
  rL LLVM

https://reviews.llvm.org/D33850