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

[Kohei Asano via Phabricator via llvm-commits]

khei4 marked an inline comment as done.
khei4 added inline comments.


================
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.
----------------
nikic wrote:
> `<=` should be `>=` here?
Right! 


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1647
+  // Can't handle unknown size alloca (e.g. VLA)
+  if (!AllocaSize)
+    return false;
----------------
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?


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


================
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();
----------------
nikic wrote:
> Then you don't need to check `!MemDepAlign`
Thank you!


================
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) {
----------------
nikic wrote:
> This case is fine, the problematic case is if the `%val` alignment is smaller.
Yeah, you seem right. Thank you for the comment! 
I also found the alive2 has similar issues :)

https://alive2.llvm.org/ce/z/NZivTQ


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

https://reviews.llvm.org/D150970