[PATCH] D73763: [DSE] Lift post-dominance restriction.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for working on this!

+1 to the general idea of "adding a read-everything intrinsic doesn't smell great," and a further +1 for containing it to only this pass if we do end up needing it.

This is the first time I'm glancing at this pass, so I'm going to err a bit on the side of overcommunication in hopes of making misunderstandings on my side more obvious. :)

I gather that `getDomMemoryDef`, given two defs A (`Current`) and B (`KillingDef`):

- Determines some Def/Phi that dominates A, C.
- Walks all of the `MemoryAccess`es that're transitively users of C, stopping at full overwrites of C + B.
- If any of those `MemoryAccess`es is a potential read of `A`, gives up. Otherwise, returns `A`

If we remove the postdom restriction, I'm uncertain about how this algo will scale. It used to be that all paths necessarily terminated at `B`, but now it looks like we'll potentially walk a ton more? e.g.,

  bool branch(); // inaccessiblememonly
  
  {
    // 1 = MemoryDef(LOE) -- DomAccess
    *a = 1;
    if (branch()) {
      // 2 = MemoryDef(1) -- The thing we want to replace it with
      *a = 2;
    }
    // 3 = MemoryPhi(2, 1)
    // Every Phi|Def below here is now walked (?)
  }

To be clear, I'm not saying this walking isn't necessary; just that it now happens AFAICT.

In any case, the thing I see this new intrinsic standing in for is an oracle that responds to "here's the set of blocks we've observed complete overwrites in. Is there any way for us to exit this function without going through one of those blocks, starting from `A->getBlock()`?" This is an expensive problem, but I think we can simplify it substantially, *if* my comment about the extra walking above is correct. I'd imagine we could do it essentially by constructing a sort-of def frontier for each `ret` terminator. In other words, a `DenseSet<MemoryAccess *> // (MemoryDef || MemoryPhi)` where membership in the set == "there exists a path where this `Def`/`Phi` is the final `Def`/`Phi` before we hit a `ret`." Since lifting the postdom restriction makes us explore every `Def`/`Phi` reachable from C (stopping at full overwrites, and stopping after N steps), we should hopefully be able to consult that set in the `getDomMemoryDef` loop to get a similar effect to the fake read-everything calls?



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1596
 
   // Find a MemoryDef writing to \p DefLoc and dominating \p Current, with no
   // read access in between or return None otherwise. The returned value may not
----------------
fhahn wrote:
> Tyker wrote:
> > the comment seems outdated.
> > i think that with this patch dominating will not be true anymore.
> > and MemoryPHI seems to not cause a bail out.
> Yes, I've rebased the patch and updated the comment as well.
(did this update get dropped accidentally? This comment still appears to say "Find a MemoryDef, ...")


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1682
       for (Use &U : Acc->uses())
         WorkList.insert(cast<MemoryAccess>(U.getUser()));
     };
----------------
tangential to this patch: should we be ignoring `U`s which are `MemoryDefs` whose only Use of `Acc` is as their `getOptimized()` operand? Without that, it's not immediately obvious to me how we aren't overly conservative in situations like:

```
{
  // 1 = MemoryDef(LOE) -- this is DomAccess
  *a = 1;
  // 2 = MemoryDef(1)
  *b = 2;
  // 3 = MemoryDef(2) -- this is the obvious kill
  *a = 3;
  // 4 = MemoryDef(3) (optimized to 2)
  *c = 4;
  // MemoryUse(4) -- AA says MayAlias for `(d, a)`, so `isReadClobber()` should be `true` if we visit this?
  if (*d) {
    // ...
  }
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73763