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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:986
+    }
+  }
+
----------------
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.)


https://reviews.llvm.org/D30225