[PATCH] D88778: [MemCpyOpt] Fix MemorySSA preservation
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 7 09:32:17 PDT 2020
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
More information about the llvm-commits
mailing list