[PATCH] D72148: [DSE] Support traversing MemoryPhis.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1642
       if (isa<MemoryPhi>(UseAccess)) {
-        LLVM_DEBUG(dbgs() << "  ...  hit MemoryPhi\n");
-        return None;
+        if (KillingDef == UseAccess)
+          continue;
----------------
fhahn wrote:
> asbirlea wrote:
> > Can you clarify this? 
> > KillingDef is a MemoryDef, UseAccess is a MemoryPhi inside this if. How can they be equal?
> I think this was a left-over before switching to a SetVector worklist. With the SetVector, we will never visit the node twice. I've added an assertion to be sure to check for cycles.
Sure, but this equality cannot happen, right? They're different types, and one if not a subtype of the other. One is a `MemoryPhi` and one a `MemoryDef` (`KillingDef`, the store that started the search), they cannot be equal even if you didn't check for cycles.

Sounds like you're thinking of not seeing `DomAccess` again as the worklist is a set, but you may see it AFAICT. If at :1617 `DomAccess` uses itself, it will be added to the worklist. I'm inclined to believe the change you meant is:
```
if (DomAccess == UseAccess)
  continue;
```
But just as well not having anything is fine. The `PushMemUses` will be a noop, as all uses have already been added.




================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1651
 
-      if (StartDef == UseAccess)
+      if (KillingDef == UseAccess)
         continue;
----------------
Should this be `Current == UseAccess` ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72148/new/

https://reviews.llvm.org/D72148