[PATCH] D153453: (WIP)[MemCpyOpt] implement single BB stack-move optimization which unify the static unescaped allocas

[Kohei Asano via Phabricator via llvm-commits]

khei4 added a comment.

Thank you for the review!



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


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


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1537
+  auto *NewStart = Builder.CreateLifetimeStart(SrcAlloca, AllocaSize);
+  NewStart->addParamAttr(1, Attribute::NoCapture);
+
----------------
nikic wrote:
> You don't need to add this manually, it is implied by the intrinsic declaration.
Thanks! Yeah, I forgot lifetimes already has them. (just for note https://github.com/khei4/llvm-project/blob/main/llvm/include/llvm/IR/Intrinsics.td#L1518-L1527 


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


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

https://reviews.llvm.org/D153453