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

[bryant via llvm-commits]

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;
----------------
bryant wrote:
> 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.
> 
Here's an example, in case the explanation wasn't clear:

```
%x = load
P   ; clobbers the load
X   ; clobbers the load and store
store %x
```

MCO will turn this into

```
X   ; clobbers the load and store
%x = load
store %x       ; merged with the load into a memcpy
P   ; clobbers the load
```

which is bad, since the load is moved past X.


Repository:
  rL LLVM

https://reviews.llvm.org/D26811