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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added a comment.

In D73763#1956347 <https://reviews.llvm.org/D73763#1956347>, @george.burgess.iv wrote:

> > I think there are 2 cases to distinguish:
> > 
> > 1. For accesses to non-alloca objects, this matches what the intrinsic achieves I think. We can only eliminate stores to objects that are visible after the function returns, if they are overwritten along all paths to the exit. So if we have determined a set of blocks that overwrite A (and there are no reads in between), we could check if all paths to the exit from A must go through one of the overwriting blocks. I think that matches your suggestion.
>
> Sounds like what I was thinking, yeah.
>
> > For objects that are not visible to the caller (e.g. alloca), the intrinsic achieves something slightly different: Given A and B as above, we can eliminate A, if there are no reads of A between A and any of the exits from A. We can stop walking any paths that contain overwrites of A.
>
> I'm confused about why `alloca`s need special treatment. This new set that we're building is essentially a fast way to answer the question "are there potentially unknowable reads after this `Def`/`Phi`?" If we know memory (e.g., from an `alloca`) is unreadable outside of the function, I'd hope we can ignore the set entirely, like this patch is doing now? Baked into my suggestion was the assumption that we were already:


Ah right, I think I was not sure if your suggestion also applied to the alloca case in the previous comment.

IIUC, the additional set would be used for the non-alloca cases, as we have to ensure that there are overwrites along all paths to the exit. For allocas, we have to explore all access along all paths to function exits, as this patch currently does. What I meant to say in my previous comment was that I think we have to stick to the walking as in the patch, which is also what your last comment said, unless I am missing something :)

> In any case, if you're confident that this works for #1, I'm content to move discussion about "why don't things work as they are now," to a patch that doesn't block this one.

The discussion so far has been extremely helpful, thanks! I think it would make sense to address the alloca/non-alloca separately. If you are happy with the direction, I would update this patch to only handle the alloca case (basically this patch modulo the intrinsic/LastDefOrPhi) and address the non-alloca in a subsequent patch.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1492
+  // Set of the last MemoryDefs or MemoryPhis before a function exit, that is
+  // defs or phis without any users that are defs or phis.
+  SmallPtrSet<MemoryAccess *, 8> LastDefOrPhi;
----------------
george.burgess.iv wrote:
> > that is defs or phis without any users that are defs or phis.
> 
> This isn't quite complete; consider:
> 
> ```
> void bar(); // inaccessiblememonly
> void foo(int *a) {
>   // 1 = MemoryDef(LOE)
>   *a = 1;
>   if (bar())
>     return;
>   // 2 = MemoryDef(1)
>   *a = 2;
> }
> ```
> 
> Despite having `2` as a `User`, we'd still want `1` to be in this set, since depending on the way the `if` goes, it's the last Def/Phi before the function ends
Of course, I remember why the intrinsic was so convenient to start with again! Anyways, I plan to split up the patch and probably look into using the suggested set straight away instead for the cases that required LastPhiOrDef.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73763