[PATCH] D153453: (WIP)[MemCpyOpt] implement inter-BB stack-move optimization which unify the lifetime overlapped fully copied allocas, the src is never modified or referenced

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1459
+  // 2. Check that src and dest are never captured, unescaped allocas.
+  if (llvm::PointerMayBeCaptured(SrcAlloca, /* ReturnCaptures=*/true,
+                                 /* StoreCaptures= */ true) ||
----------------
nit: `llvm::` prefix should not be necessary.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1469
+  SmallVector<IntrinsicInst *, 4> LifetimeMarkers;
+  // TODO: non instruction user? -> ConstantExpr. care?
+  std::optional<Instruction *> FirstUser, LastUser;
----------------
Instructions cannot have non-instruction users. You can use `cast<Instruction>`.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1470
+  // TODO: non instruction user? -> ConstantExpr. care?
+  std::optional<Instruction *> FirstUser, LastUser;
+  auto DestLoc = MemoryLocation(DestAlloca, LocationSize::precise(Size));
----------------
For pointers, don't use `std::optional`, initialize with `nullptr` instead.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1471
+  std::optional<Instruction *> FirstUser, LastUser;
+  auto DestLoc = MemoryLocation(DestAlloca, LocationSize::precise(Size));
+  bool DestMod = false, DestRef = false;
----------------



================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1476
+    if (auto *I = dyn_cast<Instruction>(U)) {
+      if (!FirstUser || I->comesBefore(*FirstUser))
+        FirstUser = I;
----------------
comesBefore() will assert if the I and FirstUser are in different blocks -- I don't think anything prevents that from happening here?


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1481
+      if (auto *II = dyn_cast<IntrinsicInst>(I);
+          II && II->isLifetimeStartOrEnd()) {
+        int64_t Size = cast<ConstantInt>(II->getArgOperand(0))->getSExtValue();
----------------
isLifetimeStartOrEnd() is defined on Instruction, so no need to cast to IntrinsicInst.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1488
+      }
+      ModRefInfo Res = AA->getModRefInfo(I, DestLoc);
+      if (I->comesBefore(Store) && isModSet(Res))
----------------
This assumes that only direct users can ModRef the alloca. Consider something like `store v, (getelementptr (alloca))`. Your loop will visit the `getelementptr` and decide it's `NoModRef`, and never see the `store`.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1507
+      if (I->comesBefore(Store) || I == Store)
+        continue;
+      if (auto *II = dyn_cast<IntrinsicInst>(I);
----------------
Don't you want to collect the lifetime intrinsics before the store as well?


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1537
+  auto *NewStart = Builder.CreateLifetimeStart(SrcAlloca, AllocaSize);
+  NewStart->addParamAttr(1, Attribute::NoCapture);
+
----------------
You don't need to add this manually, it is implied by the intrinsic declaration.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1558
+  NumStackMove++;
+  return false;
+}
----------------
`return true`. This should fix the `memcpy`s that get left behind.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1667
+  AllocaInst *DestAlloca = dyn_cast<AllocaInst>(M->getDest());
+  if (DestAlloca == nullptr)
+    return false;
----------------
nit: Use `!DestAlloca` instead. Same below.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1675
+    return false;
+  if (performStackMoveOptzn(M, M, DestAlloca, SrcAlloca, Len->getZExtValue())) {
+    // Avoid invalidating the iterator.
----------------
Pass `BAA` to the method and use it instead of `AA`.


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

https://reviews.llvm.org/D153453