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

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Jakub,

thanks for the new documentation. Just a couple of comments, most minor. This is a clear improvement and will help the discussion (and future people developing the code). Now just trying to understand where I am wrong in Daniel's example.

Best,
Tobias



================
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:
> 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.)
Should this be:

> post-dom root child *is* a return

It seems the old version slipped in again?



================
Comment at: unittests/IR/DominatorTreeTest.cpp:410
+        PostDomTree NPDT(F);
+        EXPECT_EQ(PDT->compare(NPDT), 0);
       });
----------------
nice


================
Comment at: unittests/IR/DominatorTreeTest.cpp:451
+// gets connected to the virtual exit.
+// D does not postdominate B anymore, bacause there are two forward paths from
+// B to the virtual exit:
----------------
because


================
Comment at: unittests/IR/DominatorTreeTest.cpp:455
+//  - B -> D -> VirtualExit.
 //
 TEST(DominatorTree, DeletingEdgesIntroducesInfiniteLoop) {
----------------
Thank you for the nice documentation. I still don't like that we loose the dominance relation between B and D, but at least this behavioral change is very clear.


https://reviews.llvm.org/D35851