[PATCH] D26811: [MemCpyOpt] Don't sink LoadInst below possible clobber.

[bryant via llvm-commits]

bryant marked an inline comment as done.
bryant added inline comments.


================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:619
         if (P && P != SI) {
-          if (!moveUp(AA, SI, P))
+          if (!moveUp(AA, SI, P, LI))
             P = nullptr;
----------------
efriedma wrote:
> I'm not really following what this patch is doing.  Whether it's legal to sink the load to location P isn't related to whether it's legal to hoist the store to location P.
> 
> On a side-note, this whole block of code will never run in a normal pass pipeline; "T->isAggregateType()" will never be true because instcombine breaks up aggregate loads and stores.
> I'm not really following what this patch is doing. Whether it's legal to sink
> the load to location P isn't related to whether it's legal to hoist the store
> to location P.

The store isn't necessarily the only thing to be hoisted to P. Instructions that
the store 1) depends on or 2) aliases also need to be hoisted. That's `moveUp`'s
job: To figure out which instructions, in addition to the store, need to be
hoisted.

Once all the hoisting is done, a memcpy/memmove is created to replace the load
and store. However, this implies sinking the load past the hoisted stuff to just
before the store, which may not be legal.

So this patch ensures that it's legal for the load to sink past the hoisted
stuff, even though no explicit sinking is needed.

Does that make sense?

> On a side-note, this whole block of code will never run in a normal pass
> pipeline; "T->isAggregateType()" will never be true because instcombine
> breaks up aggregate loads and stores.

InstCombine refrains from exploding aggregate loads and stores when there's
padding. I've updated the test case accordingly.



Repository:
  rL LLVM

https://reviews.llvm.org/D26811