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

Chandler Carruth via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 21 11:22:40 PDT 2017


chandlerc added a comment.

Sure, comments inline...



================
Comment at: lib/Transforms/Utils/CloneFunction.cpp:313
               SimplifyInstruction(NewInst, BB->getModule()->getDataLayout())) {
-        // On the off-chance that this simplifies to an instruction in the old
-        // function, map it back into the new function.
-        if (Value *MappedV = VMap.lookup(V))
-          V = MappedV;
+        assert((dyn_cast<Instruction>(V) == nullptr ||
+                dyn_cast<Instruction>(V)->getParent() == nullptr ||
----------------
chandlerc wrote:
> Just use isa<...> here...
Still outstanding.


================
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);
----------------
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...


Repository:
  rL LLVM

https://reviews.llvm.org/D33850





More information about the llvm-commits mailing list