[PATCH] D140089: [MemCpyOpt] Add a stack-move optimization to opportunistically merge allocas together, disabled by default.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1853-1865
+  if (std::optional<TypeSize> SrcSize =
+          SrcAlloca->getAllocationSizeInBits(DL)) {
+    if (std::optional<TypeSize> DestSize =
+            DestAlloca->getAllocationSizeInBits(DL)) {
+      SizeOK = !SrcSize->isScalable() && !DestSize->isScalable() &&
+               Size * 8 == SrcSize->getFixedSize() &&
+               Size * 8 == DestSize->getFixedSize();
----------------
Can reflow to early exit if either one is dynamic 


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1868
+  // Make sure the two allocas have the same alignment.
+  if (SrcAlloca->getAlign() != DestAlloca->getAlign()) {
+    LLVM_DEBUG(dbgs() << "Stack Move: Alignment mismatch\n");
----------------
you could adjust the alignments up


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1899-1902
+    LLVM_DEBUG(dbgs() << "Stack Move: Source was captured:");
+    if (SrcTracker.AbortingUser != nullptr)
+      LLVM_DEBUG(dbgs() << "\n" << *SrcTracker.AbortingUser);
+    LLVM_DEBUG(dbgs() << "\n");
----------------
Can merge these into one LLVM_DEBUG block


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1963-1964
+         "and destination");
+  ConstantInt *CI =
+      ConstantInt::get(Type::getInt64Ty(SrcAlloca->getContext()), Size);
+  BasicBlock *Dom =
----------------
You should query the pointer size for the pointer type


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1993
+        auto PostDomNode = (*PDT)[PostDom]->getIDom();
+        PostDom = PostDomNode == nullptr ? nullptr : PostDomNode->getBlock();
+      }
----------------
Swap condition? PostDomNode ? getBlock : null


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:2128-2130
+  if (MemCpyOptStackMoveThreshold > 0) {
+    if (AllocaInst *DestAlloca =
+            dyn_cast<AllocaInst>(M->getDest()->stripPointerCastsAndAliases())) {
----------------
More early returns


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:2130
+    if (AllocaInst *DestAlloca =
+            dyn_cast<AllocaInst>(M->getDest()->stripPointerCastsAndAliases())) {
+      if (AllocaInst *SrcAlloca = dyn_cast<AllocaInst>(
----------------
nikic wrote:
> stripPointerCasts() is sufficient. You'll never get an alloca looking through aliases.
Is there any real reason for a strip here with opaque pointers? This only does anything for addrspacecast?


================
Comment at: llvm/test/Transforms/MemCpyOpt/stack-move.ll:36
+}
+
+; Tests that the optimization succeeds with a basic call to memmove.
----------------
Add a test with a volatile memcpy. Also one that shows metadata that was attached to these


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140089