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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 4 inline comments as done.
fhahn 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;
----------------
asbirlea wrote:
> 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.
> 
> 
> Sure, but this equality cannot happen, right? 

Ah yes! I thought it was needed for a test case I added, but after the refactoring it is not necessary any longer. I've removed it


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1651
 
-      if (StartDef == UseAccess)
+      if (KillingDef == UseAccess)
         continue;
----------------
asbirlea wrote:
> Should this be `Current == UseAccess` ?
I've added a comment why this check is there. We have to make sure the KillingDef does not read the memory location. The easiest way is to just consider it as regular use, but skip the overwrite check. It probably makes more sense to just check for self reads before getDomMemoryDef. I've added a TODO.

On a related note, I've put up D75025, which should help with avoiding a number of unnecessary duplicated checks for the same killing def.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72148