[PATCH] D88778: [MemCpyOpt] Fix MemorySSA preservation

[Nikita Popov via Phabricator via llvm-commits]

nikic 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)) {
----------------
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;
  }
}
```


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

https://reviews.llvm.org/D88778