[PATCH] D78932: [DSE,MSSA] Relax post-dom restriction for objs visible after return.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Sorry for the latency :)

> I would be happy to iterate on that once we have a correct version in tree (that makes balancing the patches and benchmarking a bit easier I think), but I could also try to do it up-front, if that's preferred.

WFM.

> I think there potentially are cases that would be handled more efficiently with the current approach (e.g. if there are a large number of memoryDefs/uses to traverse)

Agreed, though again, the set-based approach only needs to be built once whereas this one needs to happen on every query, so it all depends on how many times we fall back to this codepath, how large the function is, and how sparse the Defs/Phis in the function are.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1802
+        else
+          for (BasicBlock *R : PDT.getRoots()) {
+            if (!DT.isReachableFromEntry(R))
----------------
fhahn wrote:
> george.burgess.iv wrote:
> > i'm unsure how we reach this case. AFAICT, `PDT.dominates(nullptr, AnythingElse) == false`, since the PDT should fail to lookup the node for `nullptr`, no?
> We can hit this scenario when there's no single exit block that post-dominates all killing blocks, like in the example below. Instead the special 'nullptr'
> block is used, but for starting the traversal we need to start with all exit blocks I think.
> 
> ```
> define void @test12(i32* %P) {
>   store i32 0, i32* %P
>   br i1 true, label %bb1, label %bb2
> 
> bb1:                                              ; preds = %0
>   store i32 1, i32* %P
>   br label %bb3
> 
> bb2:                                              ; preds = %0
>   store i32 1, i32* %P
>   ret void
> 
> bb3:                                              ; preds = %bb1
>   ret void
> }
> ```
Sorry, I should've been a bit more clear. Specifically, this code is guarded by `PDT.dominates(CommonPred, DomAccess->getBlock())`. `DomAccess->getBlock()` should never be `nullptr` and `PDT.dominates(nullptr, not_nullptr)` should always be `false` AFAIK, so I think we should never hit this `else` if `!CommonPred`? Or maybe we do for `DomAccess`es which are in unreachable code (but in that case, do we care to do extra work?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78932