[PATCH] D38374: [memcpyopt] Memcpy-memcpy dependence isn't detected across basic blocks

[Dan Gohman via Phabricator via llvm-commits]

sunfish marked 2 inline comments as done.
sunfish added a comment.

Thanks for taking a look!



================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:925
+
+void MemoryDependenceResults::getSpecificNonLocalPointerDependency(
+    Instruction *QueryInst,
----------------
sanjoy wrote:
> Would it be better to call this `getNonLocalPointerDependencyFrom`, in line with `getPointerDependencyFrom`?
Sure.


================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:1240
 
+  SmallVector<NonLocalDepResult, 2> NonLocalDepResults;
+  if (SrcDepInfo.isNonLocal()) {
----------------
sanjoy wrote:
> Why not push this `SmallVector` into the `if` block?
Done.


================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:1274
     if (MemSetInst *MDep = dyn_cast<MemSetInst>(SrcDepInfo.getInst()))
       if (performMemCpyToMemSetOptzn(M, MDep)) {
         MD->removeInstruction(M);
----------------
sanjoy wrote:
> IIUC, this may try to create illegal IR (def not dominating use) if it finds the non-local memset via a backedge since it tries to create a memset before the memcpy using the memset's value operand, which may not dominate the memcpy with your change.
If the memset doesn't dominate the memcpy, the non-local dependence results reflect this. For example, in
```
    for (...) {
        memcpy(out, buffer, size);
        memset(buffer, 0, size);
    }
```
the memcpy depends on the memset around the backend, but memdep reports two dependencies: the memcpy on the backedge, but also whatever there is on the loop entry edge, so the code in this patch requiring a single dependency doesn't accept it.


Repository:
  rL LLVM

https://reviews.llvm.org/D38374