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

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

Eesh -- sorry for taking forever. :)

I think my questions are answered, and seeing how this works SGTM. If we need something faster (or wanna experiment with other approaches), I think we have a solid way forward on that.

That said, LGTM. Thanks again!



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1802
+        else
+          for (BasicBlock *R : PDT.getRoots()) {
+            if (!DT.isReachableFromEntry(R))
----------------
fhahn wrote:
> fhahn wrote:
> > george.burgess.iv wrote:
> > > 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?)
> > > PDT.dominates(nullptr, not_nullptr) should always be false
> > 
> > Hm, IIUC nullptr stands for the (virtual) root node in the post dominator tree. Unless I am missing something, I think ` PDT.dominates(nullptr, not_nullptr)` should always be `true`, i.e. each node in the function is post-dominated by the (virtual) root.
> @george.burgess.iv does the above make sense?
Yup!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78932