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

[Han Zhu via Phabricator via llvm-commits]

zhuhan0 added a comment.

Sorry for the delay. I've been occupied with other projects.



================
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;
----------------
Not sure I fully understand the FIXME. Why not drop the equals case?


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1340
       return Changed;
+    } else {
+      // At this point loop may access load only for memcpy in same underlying
----------------
Nit: please fix lint.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1348
 
+  bool UseMemMove = IsMemCpy ? Verifier.IsSameObject : LoopAccessStore;
+  if (UseMemMove)
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107075