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

[Kohei Asano via Phabricator via llvm-commits]

khei4 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.
----------------
nikic wrote:
> I don't understand what this FIXME is about.
This meant adding the Instruction version isPotentiallyReachableFromMany (currently only for BasicBlocks) might be helpful. I copied the code from [[ https://github.com/khei4/llvm-project/blob/43698c1ddc179ccd97b3f3b2bb03f4a3fe9556f3/llvm/lib/Analysis/CFG.cpp#L133 | CFG.h ]]. Although even for the BasicBlocks version, a user is only a few cases. 


================
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);
----------------
nikic wrote:
> 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.
Thanks! It seems right, I added a test.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1434-1437
+    if (isa<BasicBlock *>(I1))
+      return I1;
+    if (isa<BasicBlock *>(I2))
+      return I2;
----------------
nikic wrote:
> 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).
Thanks! Sorry, I didn't notice the reverted change...


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

https://reviews.llvm.org/D155406