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

[Kohei Asano via Phabricator via llvm-commits]

khei4 added a comment.

Thank you for the review!



================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1682
+        (PDom && isModOrRefSet(AA->getModRefInfo(PDom, SrcLoc))) ? PDom
+                                                                 : nullptr;
+    if (PDom && PDom->isTerminator() && PDomUser) {
----------------
nikic wrote:
> khei4 wrote:
> > nikic wrote:
> > > khei4 wrote:
> > > > nikic wrote:
> > > > > I don't get what you're doing here with AA. Why is this necessary now and wasn't needed in the single BB case?
> > > > This was to know whether the post dominator has MemoryAccess on SrcAlloca.
> > > > If it has MemoryAccess, check whether it's terminator or not, and find the place to insert MemoryAccess.
> > > > If it's not, currently skipped because I can't know how to insert MemoryDef or MemoryPhi consistently with any post dominators.
> > > Based on D157979, you can pass nullptr as the defining access and use createMemoryAccessInBB with `Begin` insertion place (when inserting at start of block).
> > Sounds great! Thank you! I'll handle it.
> Hm, I really don't like how we distinguish between pdom being a user and the start of a block here. I think it may be better if we explicitly distinguish these two cases during the analysis.
> 
> I had something like this in mind:
> ```
> /// PDom is either an Instruction, or the start of a BasicBlock.
> using PDom = PointerUnion<Instruction *, BasicBlock *>;
> static PDom findNearestCommonPostDominator(PDom I1, PDom I2,
>                                            PostDominatorTree *PDT) {
>   auto GetParent = [](PDom I) {
>     if (auto *BB = dyn_cast<BasicBlock *>(I))
>       return BB;
>     return cast<Instruction *>(I)->getParent();
>   };
>   BasicBlock *BB1 = GetParent(I1);
>   BasicBlock *BB2 = GetParent(I2);
>   if (BB1 == BB2) {
>     if (isa<BasicBlock *>(I1))
>       return I1;
>     if (isa<BasicBlock *>(I2))
>       return I2;
>     return cast<Instruction *>(I1)->comesBefore(cast<Instruction *>(I2)) ? I2
>                                                                          : I1;
>   }
>   BasicBlock *PDomBB = PDT->findNearestCommonDominator(BB1, BB2);
>   if (!PDomBB)
>     return nullptr;
>   if (BB2 == PDomBB)
>     return I2;
>   if (BB1 == PDomBB)
>     return I1;
>   return PDomBB;
> }
> ```
> For `Instruction *` PDom you would use createMemoryAccesAfter, for `BasicBlock *` you would use `createMemoryAccessInBB`.
Yeah, I agree, it seems better to define the insertion point type as you gave. Moreover, checking BB or not could erase the ModRef check!


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

https://reviews.llvm.org/D155406