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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri 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;
----------------
zhuhan0 wrote:
> 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.
I believe, you have answered a question different from the one i asked.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97667