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

[Nikita Popov via Phabricator via llvm-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

It looks like the alignment checks are inverted.



================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1624
+///     pointer is dereferenceable for the required range
+///     2-2. The src pointer has alignment <= the alloca alignment or can be
+///     enforced so.
----------------
`<=` should be `>=` here?


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1639
+  // 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.
----------------
"Even if the arg can be based on multiple allocas" or so.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1647
+  // Can't handle unknown size alloca (e.g. VLA)
+  if (!AllocaSize)
+    return false;
----------------
`&& !AllocaSize->isScalable()` (and a test for this)


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1662
+  // If the immut argument isn't fed by a memcpy, ignore it.  If it is fed by
+  // a memcpy, check that the arg equals to memcpy dest.
+  if (!MDep || MDep->isVolatile() || AI != MDep->getDest())
----------------
"equals to" -> "equals the" or "is equal to the"


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1673
+  auto *MDepLen = dyn_cast<ConstantInt>(MDep->getLength());
+  if (!MDepLen || AllocaSize != MDepLen->getValue().getZExtValue())
+    return false;
----------------
Does that work? getZextValue() may assert for wide integers (though probably not really relevant here).


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1676
+
+  // 2-2. the memcpy source align must be smaller than the alloca's align.
+  // If not so, we check to see if we can force the source of the memcpy to the
----------------
smaller -> larger than or equal


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1679
+  // alignment we need. If we fail, we bail out.
+  MaybeAlign MemDepAlign = MDep->getSourceAlign();
+  Align AllocaAlign = AI->getAlign();
----------------
Then you don't need to check `!MemDepAlign`


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1681-1683
+  if ((!MemDepAlign || AllocaAlign < *MemDepAlign) &&
+      AllocaAlign < getOrEnforceKnownAlignment(MDep->getSource(), MemDepAlign,
+                                               DL, &CB, AC, DT))
----------------



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


================
Comment at: llvm/test/Transforms/MemCpyOpt/memcpy.ll:444
 
 ; Can't remove memcpy for VLA because of unknown size and alignment.
 define void @immut_param_vla(ptr align 4 noalias %val) {
----------------
This is not a VLA. You need to do something like `alloca i8, i64 %n`.


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


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

https://reviews.llvm.org/D150970