[llvm] [MemCpyOpt] fix incorrect handling of lifetime markers (PR #143782)

[Nikita Popov via llvm-commits]

================
@@ -1366,56 +1366,68 @@ bool MemCpyOptPass::processMemSetMemCpyDependence(MemCpyInst *MemCpy,
 
 /// Determine whether the pointer V had only undefined content (due to Def) up
 /// to the given Size, either because it was freshly alloca'd or started its
-/// lifetime.
-static bool hasUndefContents(MemorySSA *MSSA, BatchAAResults &AA, Value *V,
-                             MemoryDef *Def, Value *Size) {
-  if (MSSA->isLiveOnEntryDef(Def))
-    return isa<AllocaInst>(getUnderlyingObject(V));
-
-  if (auto *II = dyn_cast_or_null<IntrinsicInst>(Def->getMemoryInst())) {
-    if (II->getIntrinsicID() == Intrinsic::lifetime_start) {
-      auto *LTSize = cast<ConstantInt>(II->getArgOperand(0));
-
-      if (auto *CSize = dyn_cast<ConstantInt>(Size)) {
-        if (AA.isMustAlias(V, II->getArgOperand(1)) &&
-            LTSize->getZExtValue() >= CSize->getZExtValue())
-          return true;
-      }
+/// lifetime by walking the MSSA graph.
+static bool hadUndefContentsBefore(MemorySSA *MSSA, BatchAAResults &BAA,
+                                   Value *V, MemoryAccess *Clobber,
+                                   MemoryLocation Loc, Value *Size) {
+  while (1) {
+    Clobber = MSSA->getWalker()->getClobberingMemoryAccess(Clobber, Loc, BAA);
+    MemoryDef *Def = dyn_cast<MemoryDef>(Clobber);
+    if (!Def)
+      return false;
+
+    if (MSSA->isLiveOnEntryDef(Def))
+      return isa<AllocaInst>(getUnderlyingObject(V));
+
+    if (auto *II = dyn_cast_or_null<IntrinsicInst>(Def->getMemoryInst())) {
+      if (II->getIntrinsicID() == Intrinsic::lifetime_start) {
+        auto *LTSize = cast<ConstantInt>(II->getArgOperand(0));
 
-      // If the lifetime.start covers a whole alloca (as it almost always
-      // does) and we're querying a pointer based on that alloca, then we know
-      // the memory is definitely undef, regardless of how exactly we alias.
-      // The size also doesn't matter, as an out-of-bounds access would be UB.
-      if (auto *Alloca = dyn_cast<AllocaInst>(getUnderlyingObject(V))) {
-        if (getUnderlyingObject(II->getArgOperand(1)) == Alloca) {
-          const DataLayout &DL = Alloca->getDataLayout();
-          if (std::optional<TypeSize> AllocaSize =
-                  Alloca->getAllocationSize(DL))
-            if (*AllocaSize == LTSize->getValue())
+        if (Size)
+          if (auto CSize = dyn_cast<ConstantInt>(Size))
+            if (BAA.isMustAlias(V, II->getArgOperand(1)) &&
+                LTSize->getZExtValue() >= CSize->getZExtValue())
               return true;
+
+        // If the lifetime.start covers a whole alloca (as it almost always
+        // does) and we're querying a pointer based on that alloca, then we know
+        // the memory is definitely undef, regardless of how exactly we alias.
+        // The size also doesn't matter, as an out-of-bounds access would be UB.
+        if (auto *Alloca = dyn_cast<AllocaInst>(getUnderlyingObject(V))) {
+          if (getUnderlyingObject(II->getArgOperand(1)) == Alloca) {
+            const DataLayout &DL = Alloca->getDataLayout();
+            if (std::optional<TypeSize> AllocaSize =
+                    Alloca->getAllocationSize(DL))
+              if (*AllocaSize == LTSize->getValue())
+                return true;
+          }
         }
+        Clobber = Def->getDefiningAccess();
+        continue;
+      } else if (II->getIntrinsicID() == Intrinsic::lifetime_end) {
+        Clobber = Def->getDefiningAccess();
+        continue;
----------------
nikic wrote:

> The tests however look better to me, so I also am wondering if there is some way to improve MemorySSA to handle simple cases like this, where there is a noalias base pointer (such as an alloca or malloc) with only a few uses in the dominator tree before the current MemoryDef location, none of which were capturing?

At least for the basic case (memcpy from fully undef alloca), the simplest and most robust way to handle it might be to do this in InstCombine. We already have this transform there: https://github.com/llvm/llvm-project/blob/ce747a16328b2fbc365e1cb1cb01cb400c2c1b4c/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp#L3218 It handles the "only stores to alloca" case, but could be extended to also handle "only loads from alloca", we just can't have a mix of both.



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