[PATCH] D159075: [MemCpyOpt] implement forward dataflow sensitive stack-move optimization

[Kohei Asano via Phabricator via llvm-commits]

khei4 added a comment.

Thank you for the review!



================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1447
+                cast<ConstantInt>(II->getArgOperand(0))->getSExtValue();
+            Size < 0 || uint64_t(Size) == AllocaSize)
+          return true;
----------------
arsenm wrote:
> I think these are supposed to just be unsigned, no < 0 check?
It seems like there exist negative args. https://github.com/llvm/llvm-project/blob/b86d3cbc1288b4d0799746c82611d17888413aec/llvm/test/Analysis/StackSafetyAnalysis/lifetime.ll#L14 For variable length?


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1457
+        if (DL.getTypeStoreSize(SI->getValueOperand()->getType()) ==
+            (*AllocaSize).getFixedValue())
+          return true;
----------------
arsenm wrote:
> Don't need the .getFixedValue()?
No. Thanks!


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1671
 
-  if (!SrcAlloca->isStaticAlloca() || !DestAlloca->isStaticAlloca())
+  if (!SrcAlloca->isStaticAlloca() || !DestAlloca->isStaticAlloca()) {
+    LLVM_DEBUG(
----------------
arsenm wrote:
> This is redundant with getAllocationSize being nullopt
This might not be very clear(I added comment), but this was to bail out for inalloca(stacksave/stackrestore) cases like 
https://github.com/llvm/llvm-project/blob/b86d3cbc1288b4d0799746c82611d17888413aec/llvm/test/Transforms/MemCpyOpt/stack-move.ll#L1134-L1139
It seems like the size nullopt check isn't sufficient to bail out for this...


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

https://reviews.llvm.org/D159075