[PATCH] D25175: [MemCpyOpt] Optimize memcpy-memcpy dependencies more aggressively.

[Eli Friedman via llvm-commits]

efriedma added a comment.

Hoisting a memcpy past unrelated instruction is generally sound... and probably a good idea if it leads to simplifications.

This needs a lot of IR tests; I don't see any at the moment.



> MemCpyOptimizer.cpp:1002
> +
> +  // Three cases:
> +  // Case 1:

You're not counting the case where nothing mods/refs a, b, or c.  We can obviously either hoist or sink if we want to in that case, but it's not clear it's actually helpful.

> MemCpyOptimizer.cpp:1027
> +    }
> +    // identify dependencies of the memcpy that also need to moved upwards.
> +    SmallVector<Instruction *, 8> tomove, stack{M};

Might want to note that you're specifically trying to hoist the computation of M->getDest().

You're missing a check whether it's actually safe to move these instructions; they could have side-effects.

You're also missing a check to make sure you aren't hoisting a memcpy past a call which throws an exception.

> MemCpyOptimizer.cpp:1051
> +    for (auto i : tomove) {
> +      i->moveBefore(MDep);
> +      // refresh MemDep cache

You need to sort tomove so the instructions are in source order.  Or there might be a better algorithm to do this; I haven't really thought it through.

Do you actually want to move M before MDep?  I think that doesn't interact correctly with the UseMemMove case.

> MemCpyOptimizer.cpp:1088
>                      MemoryLocation::getForSource(MDep)))
>      UseMemMove = true;
>  

You probably want to check this before you start moving instructions around.

> MemCpyOptimizer.cpp:1317
> +    if (MemCpyInst *MDep = dyn_cast<MemCpyInst>(d.getInst())) {
> +      if (MDep->getDest() == M->getSource()) {
> +        return processMemCpyMemCpyDependence(M, MDep);

This looks like a duplicate of the check at the beginning of processMemCpyMemCpyDependence?

Repository:
  rL LLVM

https://reviews.llvm.org/D25175