[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)) {
----------------
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.


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

https://reviews.llvm.org/D88778