[PATCH] D86651: [MemCpyOpt] Preserve MemorySSA.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

A small note to test with EXPENSIVE_CHECKS as well, as there's extra verification there.



================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:325
+  MemoryAccess *LastAcc = nullptr;
+  MemoryAccess *LastDef = nullptr;
   for (++BI; !BI->isTerminator(); ++BI) {
----------------
I'm not sure I fully understand how these two work.
`LastDef` is the last definition, last access, or the defining access for the new MemoryAccess created below. It should be always updated in the loop. Should it also be updated when BI is not a store or memset but the access is a MemoryDef?
`LastAcc` is the insertion point (perhaps rename this one?)


The insertion point for the memset in the BB, seems to be where BI left off, so then `LastAcc` is either the same as `LastDef` or one before, so it should be updated more often than just for non-store and non-memset?

If my understanding is incomplete, could you please explain?

Unrelated, below, in Ranges, the StartInst is also added and I think that's a store. So `LastAcc` could be initialized to `MSSAU->getMemorySSA()->getMemoryAccess(StartInstI);` outside the loop?





================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1091
+    auto *NewAccess = MSSAU->createMemoryAccessBefore(
+        NewM, LastDef->getDefiningAccess(), LastDef);
+    MSSAU->insertDef(cast<MemoryDef>(NewAccess), /*RenameUses=*/true);
----------------
Can you add a comment explaining this?
`The new memset is inserted after the memcpy, but it is known that its defining access is the memset about to be removed which immediately precedes the memcpy. `


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86651