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

[Eli Friedman via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM with the testcase fixed... but please don't commit changes to this code unless you're intending to write a followup which actually makes it useful.  (If you're going to leave it in its current state, we might as well just delete it.)



================
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:
> 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.
Oh, wait, I see, I was misunderstanding how the algorithm works (I somehow thought one of the loops was iterating the wrong way).  Yes, your approach makes sense.


================
Comment at: test/Transforms/MemCpyOpt/load-store-to-memcpy.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -basicaa -scoped-noalias -evaluate-aa-metadata -memcpyopt -S %s 2>/dev/null | FileCheck %s
+
----------------
evaluate-aa-metadata doesn't do anything here.


Repository:
  rL LLVM

https://reviews.llvm.org/D26811