[PATCH] D97667: [loop-idiom] Hoist loop memcpys to loop preheader

[Han Zhu via Phabricator via llvm-commits]

zhuhan0 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:868-870
+  // Check if the load stride matches the store stride.
+  if (StrIntStride != LoadIntStride && StrIntStride != -LoadIntStride)
+    return false;
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > This doesn't make sense. Strides of load and store must match exactly.
> > Doesn't this miscompile the case where load goes forward and store backward or vice verse?
> Also, i would like to see better test coverage:
> 1. negative stride
> 2. load/store stride sign mismatch 
> This doesn't make sense. Strides of load and store must match exactly.
> Doesn't this miscompile the case where load goes forward and store backward or vice verse?

The strides of load and store can be different if the memcpy source and destination are of different types. See my test `memcpy-intrinsic-different-types.ll`, which was derived from the build failure we saw earlier. I printed out `LoadIntStride` and `StoreIntStride`:
```
LoadIntStride: 32
StoreIntStride: 12
```
Line 869-870 check for this case and fix the build failure.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:868-870
+  // Check if the load stride matches the store stride.
+  if (StrIntStride != LoadIntStride && StrIntStride != -LoadIntStride)
+    return false;
----------------
zhuhan0 wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > This doesn't make sense. Strides of load and store must match exactly.
> > > Doesn't this miscompile the case where load goes forward and store backward or vice verse?
> > Also, i would like to see better test coverage:
> > 1. negative stride
> > 2. load/store stride sign mismatch 
> > This doesn't make sense. Strides of load and store must match exactly.
> > Doesn't this miscompile the case where load goes forward and store backward or vice verse?
> 
> The strides of load and store can be different if the memcpy source and destination are of different types. See my test `memcpy-intrinsic-different-types.ll`, which was derived from the build failure we saw earlier. I printed out `LoadIntStride` and `StoreIntStride`:
> ```
> LoadIntStride: 32
> StoreIntStride: 12
> ```
> Line 869-870 check for this case and fix the build failure.
> Also, i would like to see better test coverage:
> 1. negative stride
> 2. load/store stride sign mismatch 

This is reasonable. I'll add those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97667