[PATCH] D155406: [MemCpyOpt] implement multi BB stack-move optimization

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1582
+      // Instructions reachability checks from CFG analysis
+      // FIXME: if it will save many case, add isPotentiallyReachableFromMany
+      // for instructions on CFG analysis.
----------------
I don't understand what this FIXME is about.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1667
+    // meaningless.
+    // If the PDom is the terminator(e.g. invoke), see the next immediate post
+    // dominator.
----------------
nit: Space before `(`


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1677
+      if (auto *PDomI = dyn_cast<Instruction *>(PDom)) {
+        // If PDom is Instruction ptr, insert after it, because it's user of
+        // SrcAlloca.
----------------
user -> a user


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1685
+        auto *PDomB = cast<BasicBlock *>(PDom);
+        Builder.SetInsertPoint(PDomB, PDomB->begin());
+        auto *End = Builder.CreateLifetimeEnd(SrcAlloca, AllocaSize);
----------------
This should use `PDomB->getFirstInsertionPoint()`. Can you add a test where we need to insert in a block with phi nodes? In that case, the insertion has to be after the phi nodes, not before.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1434-1437
+    if (isa<BasicBlock *>(I1))
+      return I1;
+    if (isa<BasicBlock *>(I2))
+      return I2;
----------------
khei4 wrote:
> nikic wrote:
> > I think I got these the wrong way around.
> > 
> > If `I1` is the start of the block, then `I2` is either also the start of the block or comes later in it, so it post-dominates.
> Ah, thanks! This part only seems wrong(normal dominator rather than post). I added the test to check this, also.
This is still the wrong way around (and your new test multi_bb_pdom_test0 does show it: The lifetime.end is before the final use).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155406/new/

https://reviews.llvm.org/D155406