[PATCH] D150970: [MemCpyOpt] Use memcpy source directly if dest is known to be immutable from attributes

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1647
+  // Can't handle unknown size alloca (e.g. VLA)
+  if (!AllocaSize)
+    return false;
----------------
khei4 wrote:
> nikic wrote:
> > `&& !AllocaSize->isScalable()` (and a test for this)
> > && !AllocaSize->isScalable()
> 
> Sorry, I'm not sure about a scalable vectors pointer could be transformed (maybe `||`, not `&&`?). Do you mean scalable vectors TypeSize are available, but memcpy length cannot be scalable for it, so we couldn't handle it, right?
Yes, exactly.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1630
+  // 2. Check that arg is alloca
+  // TODO: Even if the arg get back to branches, we can remove memcpy if the all
+  // alloca alignments can be enforced to source alignment.
----------------
the all -> all the


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1655
+  // a memcpy, check that the arg equals the memcpy dest.
+  if (!MDep || MDep->isVolatile() || AI != MDep->getDest())
+    return false;
----------------
Add test with volatile memcpy.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1673
+  Align AllocaAlign = AI->getAlign();
+  if ((MemDepAlign < AllocaAlign) &&
+      getOrEnforceKnownAlignment(MDep->getSource(), AllocaAlign, DL, &CB, AC,
----------------
Can drop the extra `()` here.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1696
+
+  // Otherwise we're good!  Update the byval argument.
+  CB.setArgOperand(ArgNo, MDep->getSource());
----------------
byval -> immut


================
Comment at: llvm/test/Transforms/MemCpyOpt/memcpy.ll:467
 ;
   %val1 = alloca %sv
   call void @llvm.memcpy.p0.p0.i64(ptr align 4 %val1, ptr align 4 %val, i64 2, i1 false)
----------------
I'd just write `alloca <vscale x 2 x i32>` here. You're using a "homogeneous scalable aggregate" which also tests the right thing, but is a more advanced feature...


================
Comment at: llvm/test/Transforms/MemCpyOpt/memcpy.ll:473
 
 ; Can't remove memcpy because src is modified between call and memcpy
 define void @immut_param_modified(ptr align 4 noalias %val) {
----------------
We also need a test where dst is modified between call and memcpy, unless I missed it.


================
Comment at: llvm/test/Transforms/MemCpyOpt/memcpy.ll:503
 
 ; Can't remove memcpy if we remove, the alignment assumption would break
 define void @immut_param_bigger_align(ptr align 16 noalias %val) {
----------------
This comment is outdated.


================
Comment at: llvm/test/Transforms/MemCpyOpt/memcpy.ll:527
   ret void
 }
 
----------------
We're missing a test where the alignment can be enforced. For that you want to copy from another alloca with lower alignment, and the alignment will be increased by the transform.


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

https://reviews.llvm.org/D150970