[PATCH] D22568: [DSE] Implement dead store elimination using MemorySSA (disabled by default).

[Daniel Berlin via llvm-commits]

dberlin added inline comments.

================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1156
@@ +1155,3 @@
+
+  // Iterate over all local memory accesses.
+  auto *BlockAccesses = MSSA->getBlockAccesses(F->getParent());
----------------
dberlin wrote:
> I'm a bit unclear  what this is trying to do, so i can't definitely give advice, only guess.
> 
> If you are trying to see if the only use of a store is a call to free, the following applies:
> 
> Unlike loads, store uses/defs are not guaranteed to may-alias the store (doing so requires allowing multiple phi nodes in memoryssa).
> 
> If they were, this would simply be "Process instructions in reverse order, worklist the uses of the store, if all of them are in calls to free, eliminate the store".
> 
> However, because of this, you want to do:
> 
> Process instructions in reverse order:
> 
>   <worklist the initial uses of the MemoryDef for the store>
>   while(worklist not empty) 
>     Pull use off worklist.
>     If use is free call, ignore it.
>     If use is below free call (IE  MSSA->dominates(memoryaccess for free, memoryaccess for use)), ignore it.
>     (there are partially dead cases this will ignore, but let's ignore that for now)
>    Otherwise:
>      If use is a MemoryUse, you cannot remove the store (we already eliminated all uses after the free call above, and your other so this must be between the store and the free call. Both your original code and this code presume that you have already eliminated loads of just stored pointers, etc, so they don't get in the way ).
>      If use is a MemoryDef, and getClobberingMemoryAccess(use) == original store you have store over store to the same data (IE partial or full overwrite). Not sure what you want to do here.
>      If use is a MemoryDef, and getClobberingMemoryAccess(use) != original store, put the uses of this MemoryDef on worklist and continue.
>     If use is a MemoryPhi, put uses of this MemoryPhi on worklist and continue.
> 
> This seems complicated, but is still going to be faster than what you have :)
> 
(Note that the above starts from the store, instead of starting from the free, and that worklist must be a queue to get ordering right)

You can easily extend the above to be global, as well,in the sense that it "does not care where the free call is" - in this block or not.  

```

<worklist the initial uses of the MemoryDef for the store>
while(worklist not empty) 
  Pull use off front of worklist.
  If use is free call, put it on list of free calls.  
  If use is dominated by *any* free call in the list, ignore it (because it means we will hit a free before we hit that use, and thus the use is use-after-free)
  (there are partially dead cases this will ignore, but let's ignore that for now)
  Otherwise:
   If use is a MemoryUse, you cannot remove the store (we already eliminated all uses after the free call above, and your other so this must be between the store and the free call. Both your original code and this code presume that you have already eliminated loads of just stored pointers, etc, so they don't get in the way ).
   If use is a MemoryDef, and getClobberingMemoryAccess(use) == original store you have store over store to the same data (IE partial or full overwrite). Not sure what you want to do here.
   If use is a MemoryDef, and getClobberingMemoryAccess(use) != original store, put the uses of this MemoryDef on worklist and continue.
   If use is a MemoryPhi, put uses of this MemoryPhi on worklist and continue.

```

The above will handle:

IE

```
store A

if (B)
  free A
else
  free A

```

Because it's a queue, and defs dominate uses, you are guaranteed it will process it in the right order.  (IE even if you add a use(A) at the end it will get the right answer)

As an optimization, you can drop free calls from the list of things to check in some cases.  Probably not worth doing since normal code will likely have 1 or 2 frees to track.



https://reviews.llvm.org/D22568