[PATCH] D30225: [LIR] re-enable generation of memmove with runtime checks

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:986
+    }
+  }
+
----------------
efriedma wrote:
> kparzysz wrote:
> > efriedma wrote:
> > > kparzysz wrote:
> > > > efriedma wrote:
> > > > > This whole worklist thing looks very suspicious; for the purpose of figuring out whether the memmove covers the loop, why do you need to special-case the pointer operand of the load instruction?
> > > > Could you explain the "special-casing" comment?  I'm not seeing what you are referring to.
> > > Suppose, for example, you have something like this:
> > > 
> > >     ; Assume this is a loop, where %p and %q are induction variables.
> > >     %p2 = call i8* @foo(i8* returned %p)
> > >     %v = load i8* %p2
> > >     store i8 %v, i8* %q
> > > 
> > > You start off with the load and store on the worklist, add the call to the worklist, then conclude the loop is covered even though the call could have other side-effects.
> > > 
> > > Granted, that's probably unlikely to happen in practice, but I'm not sure what this loop is trying to accomplish.
> > The other side-effects should be checked elsewhere.  This wouldn't be a strided load, so it shouldn't even get this far.
> > The other side-effects should be checked elsewhere.
> 
> Elsewhere, where?  Isn't the point of coverLoop to check for side-effects?
> 
> > This wouldn't be a strided load, so it shouldn't even get this far.
> 
> It's a strided load because of the "returned" attribute on the parameter call.  (IIRC, SCEV doesn't actually look through calls like this at the moment, but it could.)
The point of this loop is to see if the loop has other stuff in it besides the load and the store, and any instructions that are only used by the load and the store.  In other words, if we removed the load and the store, and all instructions that would become recursively dead, would this loop still have anything left in it.  If not, it means that the code used (only) by the load/store covers the loop entirely.

The side-effects of the code related to the load/store should be checked somewhere else, but not here.  If it's not checked, the checks should be added.


https://reviews.llvm.org/D30225