[PATCH] D153453: [MemCpyOpt] implement single BB stack-move optimization which unify the static unescaped allocas
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 11 02:34:38 PDT 2023
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
More information about the llvm-commits
mailing list