[PATCH] D155406: [MemCpyOpt] implement multi BB stack-move optimization
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Aug 20 03:04:53 PDT 2023
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
More information about the llvm-commits
mailing list