[PATCH] D88778: [MemCpyOpt] Fix MemorySSA preservation

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:547
+  MemoryDef *PDef = MSSAU
+      ? cast<MemoryDef>(MSSAU->getMemorySSA()->getMemoryAccess(P)) : nullptr;
   for (auto *I : llvm::reverse(ToLift)) {
----------------
nikic wrote:
> asbirlea wrote:
> > nikic wrote:
> > > nikic wrote:
> > > > asbirlea wrote:
> > > > > Is it possible for the cast here to fail?
> > > > > P is known to modify the Load location a the callsite. Is it possible this is true for specific cases not modeled as MemoryDefs in MemorySSA (e.g. when using a non-standard aa pipeline, and basicaa is disabled, certain accesses are seen as "can access/modify" but are ignored in the MemorySSA modeling - see MemorySSA.cpp:1766).
> > > > I wasn't able to come up with a case that fails based on some quick experiments -- anything I tried would get foiled by the above safety checks without basicaa. However, it doesn't cost us anything to be conservative here, so I changed this to use a dyn_cast_or_null.
> > > Actually, thinking about this again, just using dyn_cast_or_null here isn't safe: Just because we can't get the MemoryDef for P does not mean that we don't have to move accesses in MemorySSA, we just no longer know where they need to be moved to. With that in mind, I think it is safer to leave the cast<> and let is assert if it turns out that we can run into this issue after all.
> > Agreed, we'd still need an insertion point.
> > I think the logic would be something like this:
> > ```
> > MemoryAccess *InsertPoint = nullptr;
> > Instruction *LastToMove = ToLift[ToList.size()-1];
> > for (auto &Ins : make_range(P->getIterator(), --LastToMove->getIterator())) {
> >   if (InsertPoint = MSSAU->getMemorySSA()->getMemoryAccess(&Ins))
> >     break;
> > }
> > 
> > ////// below
> > 
> > // No need to move accesses is InsertPoint == nullptr
> > if (MSSAU && InsertPoint) {
> >   if (MemoryUseOrDef *MA = MSSAU->getMemorySSA()->getMemoryAccess(I))
> >     MSSAU->moveBefore(MA, InsertPoint);
> > }
> > ```
> This doesn't look quite right to me either. Imagine we have this kind of sequence:
> 
> ```
> P
> ToLift[1]
> MA
> ToLift[0] = SI
> ```
> 
> While there is no memory access between P and ToLift[1], it is still necessary to move the ToLift[0] access above MA.
> 
> I think we would have to do something like this to handle it:
> 
> ```
> MemoryAccess *InsertPoint =
>     MSSAU ? MSSAU->getMemorySSA()->getMemoryAccess(P);
> // Below
> if (MSSAU) {
>   if (MemoryUseOrDef *MA = MSSAU->getMemorySSA()->getMemoryAccess(I)) {
>     if (InsertPoint)
>       MSSAU->moveBefore(MA, InsertPoint);
>     else
>       InsertPoint = MA;
>   }
> }
> ```
You're right, there may be accesses that are interleaved with ToLift entries.
But not all entries in ToList can have an access based on the check below (or is that not the case?).
For example:
```
P
ToLift[1]      -> getMemAccess = nullptr
MA
ToLift[0] = SI  -> needs to be moved before MA, MA needs to be found
```

In your example, ToLift[0] would be moved before ToLift[1], and that doesn't look right.

A simpler approach may be:
- get first access before P (if P has an access get the iterator for memory accesses and do a decrement; if it doesn't walk instructions up to LI, which has an access for sure, or pass this info into the method from the call site which does this exact walk).
- in the loop below do:
```
assert (MemInsertPoint && "Exists");
if (MemoryUseOrDef *MA = MSSAU->getMemorySSA()->getMemoryAccess(I))
  MSSAU->moveAfter(MA, MemInsertPoint);
  MemInsertPoint = MA;
}
```


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

https://reviews.llvm.org/D88778