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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Looks basically good to me.



================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1491
+  Worklist.reserve(MaxUsesToExplore);
+  SmallSet<const Use *, 20> Visited;
+
----------------
At least Visited needs to be part of CaptureTrackingWithModRef, otherwise it will be shared between the two walks. I think it would be best to move Worklist into it as well.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1494
+  auto CaptureTrackingWithModRef =
+      [&](function_ref<bool(Instruction * UI)> ModRefCallback) -> bool {
+    while (!Worklist.empty()) {
----------------
nit: No space after `*`


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1545
+
+  // 3. Check that dest has no Mod, except full size lifetime intrinsics, from
+  // the alloca to the Store.
----------------
no Mod/Ref


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1567
+
+  // 3. Check that, from the after the Load to the end of the BB,
+  // 3-1. if the dest has any Mod, src has no Ref, and
----------------
the after the -> after the


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1594
+  // Merge the two allocas.
+  DestAlloca->replaceAllUsesWith(SrcAlloca);
+
----------------
We should also erase DestAlloca.


================
Comment at: llvm/test/Transforms/MemCpyOpt/stack-move.ll:18
 
 declare i32 @use_nocapture(ptr noundef nocapture)
 declare i32 @use_maycapture(ptr noundef)
----------------
You should be able to drop all the noundef attributes everywhere, they are not relevant to the transform.


================
Comment at: llvm/test/Transforms/MemCpyOpt/stack-move.ll:118
   %src = alloca %struct.Foo, align 8
   %dest = alloca %struct.Foo, align 4
   call void @llvm.lifetime.start.p0(i64 12, ptr nocapture %src)
----------------
Shouldn't the alignments here be swapped, so you can see the alignment increase on %src after the transform?


================
Comment at: llvm/test/Transforms/MemCpyOpt/stack-move.ll:616
 
-; TODO: Prevent this transformation
 ; Tests that failure because copy semnatics will change if dest is replaced with src.
 define void @mod_dest_before_copy() {
----------------
semantics


================
Comment at: llvm/test/Transforms/MemCpyOpt/stack-move.ll:713
 
 ; Tests that the optimization isn't performed when the source and destination
 define void @src_ref_dest_mod_after_copy() {
----------------
Missing end of sentence?


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

https://reviews.llvm.org/D153453