[PATCH] D107075: [LoopIdiom] Transform loop containing memcpy into memmove

[Dawid Jurczak via Phabricator via llvm-commits]

yurai007 marked 2 inline comments as done.
yurai007 added a comment.

Rebased and addressed comments.



================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1204-1208
+      // for negative stride. FIXME: since llvm.memcpy dest and src may be equal
+      // extra check is needed here.
+      if ((!NegStride && LoadOff <= StoreOff) ||
+          (NegStride && LoadOff >= StoreOff))
+        return false;
----------------
zhuhan0 wrote:
> Not sure I fully understand the FIXME. Why not drop the equals case?
Right, apparently I didn't think enough about that case so left fixme without good reason.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1348
 
+  bool UseMemMove = IsMemCpy ? Verifier.IsSameObject : LoopAccessStore;
+  if (UseMemMove)
----------------
zhuhan0 wrote:
> I feel the logic here is a bit hard to read. Whether it's memcpy or not, it both requires `IsSameObject` to be true. Maybe change this to
> ```
> bool UseMemMove = IsMemCpy || LoopAccessStore;
> ```
> And then move the `IsSameObject` check into MemmoveVerifier?
I agree logic is not very straightforward here. Your idea would work except isMemcpy and !IsSameObject case (like in memcpy-intrinsic.ll).
For that case we just want to hoist body memcpy to big header memcpy without reaching MemmoveVerifier. 
We could still move some checks there to make main logic more readable but then that wouldn't be "MemmoveVerifier" anymore I guess. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107075