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

[Han Zhu via Phabricator via llvm-commits]

zhuhan0 marked 4 inline comments as done.
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:
> 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.
You're right. That was a silly error. Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97667