[llvm] [MemCpyOpt] allow more memcpy-to-memcpy optimization (PR #150792)

[Nikita Popov via llvm-commits]

================
@@ -1508,21 +1532,33 @@ bool MemCpyOptPass::performStackMoveOptzn(Instruction *Load, Instruction *Store,
     return false;
   }
 
-  // Check that copy is full with static size.
-  const DataLayout &DL = DestAlloca->getDataLayout();
-  std::optional<TypeSize> SrcSize = SrcAlloca->getAllocationSize(DL);
-  if (!SrcSize || Size != *SrcSize) {
-    LLVM_DEBUG(dbgs() << "Stack Move: Source alloca size mismatch\n");
+  if (SrcAlloca->isUsedWithInAlloca() || DestAlloca->isUsedWithInAlloca())
     return false;
-  }
+
+  const auto &DL = SrcAlloca->getDataLayout();
+  // Check if allocation sizes are compatible with compile-time math
+  std::optional<TypeSize> SrcSize = SrcAlloca->getAllocationSize(DL);
   std::optional<TypeSize> DestSize = DestAlloca->getAllocationSize(DL);
-  if (!DestSize || Size != *DestSize) {
-    LLVM_DEBUG(dbgs() << "Stack Move: Destination alloca size mismatch\n");
+  if (!SrcSize || !DestSize)
     return false;
-  }
+  if (*SrcSize != *DestSize)
+    if (!SrcSize->isFixed() || !DestSize->isFixed())
+      return false;
 
-  if (!SrcAlloca->isStaticAlloca() || !DestAlloca->isStaticAlloca())
-    return false;
+  // Check that copy is full with dest size, either because it wrote every byte,
+  // or it was fresh.
+  if (Size != *DestSize)
+    if (!allOverreadUndefContents(MSSA, Store, BAA)) {
----------------
nikic wrote:

> Note that `DestModRefCallback` looks like it doesn't technically exactly check that, but rather checks that there is no prior ModRef on Dest _except for this copy operation_. The difference between those could be important if the operation is in a loop and the size is less than the full dest and either the size or the offset is dynamic. The call to allOverreadUndefContents prevents that causing incorrect results in this PR, though an equivalent check could perhaps be integrated directly into DestModRefCallback instead.

This is a great point. I had not considered the loop case.

> A big part of my goal with this PR was to figure out ways to reduce the random differences between the various passes in this file (e.g. none of the other passes arbitrarily bail out because of `!isStaticAlloca`–that is already covered by the no-capture information). The use of a custom `DestModRefCallback` here and exact size checks vs. MemorySSA + overreadUndefContents everywhere else seemed like a smell.
>
> It seems like the `DestModRefCallback` design might be a superior design to the MemorySSA checks used elsewhere in this file? In particular, the MemorySSA seems to be very expensive to walk and bails too easily because it has to check every Mod statement (up to an arbitrary cutoff), instead of analyzing only those statements relevant to the alloca, but analyzing them in full. Would you consider reviewing a total replacement for the MemorySSA design in this file with an analysis instead based on this seemingly more robust and efficient DestModRefCallback design? It'll take some time, but I can add it to my backlog of improvements to attempt, if so.

A key difference between this optimization and all the other ones is that it is specific to allocas. This kind of approach where we visit all uses is generally only valid for allocas (and noalias args I guess). In other cases we need AA/MSSA to reason about memory effects. So I don't think it will be possible to use this approach elsewhere.

As to what to do for this opt, I'm not sure. Just improving the existing DestModRefCallback code to account for the "partial store inside loop" case would be an option. One advantage of using an MSSA clobber query instead of DestModRefCallback is that it would be able to distinguish between the case where you have the lifetime.start inside the loop and outside the loop. Doing the same distinction in the use-def walk would be tricky. Of course, the downside is that MSSA is slower and has a cutoff, so it may well be worse overall in practice.

https://github.com/llvm/llvm-project/pull/150792