[PATCH] D86651: [MemCpyOpt] Preserve MemorySSA.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:325
+  MemoryAccess *LastAcc = nullptr;
+  MemoryAccess *LastDef = nullptr;
   for (++BI; !BI->isTerminator(); ++BI) {
----------------
asbirlea wrote:
> 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?
> 
> 
> 
> 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?)

I renamed `LastAcc` to `MemInsertPoint`.

> 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?

Yes, `MemInsertPoint (LastAcc)` should be updated on each iteration. I updated the code. I also added comments explaining in more detail what both variables track. `MemInsertPoint` is either `LastMemDef` or the last memory user of before the insertion point for the new memsets.

> 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?

Yes I think it could be initialized with `StartInstI`, but it is set in each loop iteration now and should be much clearer, so I don't think it is necessary to additionally initialize it outside the loop 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86651