[PATCH] D40480: MemorySSA backed Dead Store Elimination.

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added inline comments.


================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1324
+      // Any MemoryUse's that may alias is a break. Otherwise ignore
+      if (AA.getModRefInfo(MA->getMemoryInst(), SILoc) & MRI_Ref)
+        return {NextResult::Bad, nullptr};
----------------
Thinking about this not too hard, my initial thought is that this should be unnecessary, IMHO. I'm curious what's happening if it's not.

Assuming we did not hit the optimization limits during memory ssa use building, any use points to a store that may-or -must aliases it.
(if we hit the opt limits, you probably want to respect them anyway :P).

So this check should just be flat out unnecessary when current ==  original access.  If that has a memory use it's not dead on the path to the memoryuse (ignoring PDSE , that means it's not dead overall)

When walking from def to def, (IE when current != originalaccess),  that leaves two cases:
The store it's a use for may-aliases your original access's siloc - your store is not dead
the store it's a use for must-aliases your original access's siloc - your stores are the same (and so maybe one is dead)
Note that in either case, what the use is for or aliases is irrelevant.  The only thing relevant to next access is the store aliasing.  The uses force your store alive or not based on what store they are linked to and it's equivalence to your original one.

Put another way: I can't see a reason to check  check explicitly whether the use aliases you.  You only have to check whether use->getDefiningAccess->memloc mayalias siloc.
MemorySSA has already checked whether use mayalias def for you.

You can cache that, and now your walk is much faster because you only have to know things about the defs and not the uses.
 
Again, i could be completely wrong, but i'd like to understand why :)

It's true that aliasing is not transitive in theory, but in llvm, the vast majority of cases where it isn't have metadata attached and you could only check here in those cases if you are worried about catching every case.



================
Comment at: lib/Transforms/Scalar/DeadStoreElimination.cpp:1689
+    // Limit on the number of memory instruction we can look at
+    int InstructionLimit = MemorySSAMemoryAccessScanLimit;
+    // Used by PartialEarlierWithFullLater to ensure that we were free of
----------------
FWIW: You can do better than this, but it's complicated.

We could build a form with factored stores and multiple phis/sigmas if it is really worth it.

You also are going to end up rediscovering the same data here again and again (IE for each store, because you are working backwards and  they are all linked, you will eventually hit the same use chain you just saw.  for a given memloc or anything that is mustalias of that memloc, the answers must be the same) . There are various hashing /pair/etc schemes you can use to cache answers.  Not sure it is worth it.

Especially vs building a real form for stores.


https://reviews.llvm.org/D40480