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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin 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);
+
----------------
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?



================
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';);
----------------
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.





https://reviews.llvm.org/D35851