[PATCH] D150970: (WIP) [MemCpyOpt]remove memcpy on immutable arguments from attributes

[Kohei Asano via Phabricator via llvm-commits]

khei4 added a comment.

@nikic 
Thanks for the review! Too much obsolete comments!



================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1636-1642
+  // TODO: collect possible allocas like
+  // %val1 = alloca [4 x i8], align 4
+  // %val2 = alloca [16 x i8], align 4
+  // %val3 = select i1 %c, ptr %val1, ptr %val2
+  AllocaInst *AI = dyn_cast<AllocaInst>(ImmutArg->stripPointerCasts());
+  if (!AI)
+    return false;
----------------
I decided to postpone to the next Revision to collect all possible pointers and check that all ptr are alloca. (it might be a little conservative.)


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1693
+  // It would be invalid to transform the second memcpy into foo(*b).
+  // Also verify source is not modified by alias.
+  if (writtenBetween(MSSA, BAA, MemoryLocation::getForSource(MDep),
----------------
nikic wrote:
> "not modified by the call" maybe?
> "not modified by the call" maybe?

Literally right,(`writtenBetween` guarantees so). TBH, I want to ensure the memcpy source is not modified by another alias during the call.
That part may be done by isModSet I guess.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1695
+  if (writtenBetween(MSSA, BAA, MemoryLocation::getForSource(MDep),
+                     MSSA->getMemoryAccess(MDep), MSSA->getMemoryAccess(&CB)) ||
+      writtenBetween(MSSA, BAA, MemoryLocation::getForDest(MDep),
----------------
nikic wrote:
> `MSSA->getMemoryAccess(&CB)` is `CallAccess`.
Good catch!! The same goes for ByVal.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1698
+                     MSSA->getMemoryAccess(MDep), MSSA->getMemoryAccess(&CB)) ||
+      isModSet(AA->getModRefInfo(&CB, MemoryLocation::getForDest(MDep))))
+    return false;
----------------
nikic wrote:
> Hm, shouldn't this be checking the source? We already know the dest is not modified via readonly noalias.
Right! Thanks, I was confused with them.


================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1708
+    TmpCast = TmpBitCast;
+  }
+
----------------
nikic wrote:
> You can drop the bitcast code, not needed with opaque pointers.
Thanks! I believe ByVal has it too! Including other comments I'll fix them on ByVal side also!


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

https://reviews.llvm.org/D150970