[PATCH] D35851: [Dominators] Include infinite loops in PostDominatorTree

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: include/llvm/Support/GenericDomTreeConstruction.h:401
+    // Step #3: If we found some non-trivial roots, make them non-redundant.
+    if (HasNonTrivialRoots) RemoveRedundantRoots(DT, Roots);
+
----------------
dberlin wrote:
> Can't you use the forward dfs in/outnumbers  to tell if the roots were redundant?
> 
> A redundant root should be within the in/out of some other root, no?
> 
It can happen that a path from a redundant root to some other one goes through reverse-reachable regions, so I think that would mean that we'd have to run the forward DFS on the whole CFG, including the forward-unreachable parts. Then, can you always tell if a node is a part of the same SCC as some other one using only DFS in/out numbers?


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:265
+    if (isa<ReturnInst>(Info.Terminator)) {
+      DEBUG(dbgs() << "post-dom root child is not a return: " << BB->getName()
                    << '\n';);
----------------
dberlin wrote:
> kuhar wrote:
> > kuhar wrote:
> > > dberlin wrote:
> > > > kuhar wrote:
> > > > > @dberlin: shouldn't this be:
> > > > > > post-dom root child *is* a return
> > > > > 
> > > > > ?
> > > > > 
> > > > > 
> > > > yes, it should be.
> > > Followup: why do we only check for ReturnInst here? I mean, shouldn't we treat UnreachableInst the same way and not mark it as live? 
> > I checked and continuing also on UnreachableInst doesn't seem to cause any regressions.
> I'm actually curious if ADCE works without this loop at all now
> In theory, it claims it was being done "to protect IDF calculator" (which is definitely not right, but ...).
> It already knows to mark loops live if !removeloops.  Everything else should be fair game to remove, at least by the current logic.
> 
> But i think we should worry about this in a followup, and stick with making the original loop do "the same thing"on the postdom tree it was trying to do before.
> 
> 
> 
Yeah, I think that it'd be reasonable to revisit it later.
(BTW, I tried deleting the loop, but it resulted in multiple things crashing/failing.)


https://reviews.llvm.org/D35851