[llvm] r296535 - Fix PR 24415 (at least), by making our post-dominator tree behavior sane.

Thu Mar 2 12:25:07 PST 2017

On Thu, Mar 2, 2017, at 09:20 PM, Daniel Berlin via llvm-commits wrote:
> I'm not sure when I'll next be near git (it's a bit hard to predict on
> this
> vacation) so please feel free to revert it.  Thought I mentioned that but
> looking back, I did not.  If that doesn't work, let me know and I'll ping
> someone to do it.

Happy to do so. Just wanted to not start a commit fight. Thanks for
being so understanding. I am looking forward to an insightful,
productive, and hopefully not too long discussion. Enjoy your vacations!

Best,
Tobias

> 
> On Thu, Mar 2, 2017, 12:10 PM Tobias Grosser <tobias.grosser at inf.ethz.ch>
> wrote:
> 
> > On Thu, Mar 2, 2017, at 07:54 PM, Daniel Berlin wrote:
> > > (and, fwiw, i wrote this in a rush because i'm on vacation and running
> > > around, but wanted to make sure i responded.
> > > I didn't edit it very hard, so please don't assume I mean anything mean
> > > by
> > > saying "non-standard", etc, in a bunch of places :P)
> >
> > Hi Dani,
> >
> > I don't think there is a clear disagreement yet, I mostly want to get
> > time to carefully think about all implications. I know you have (and
> > there are) good reasons and arguments to go this way. Still, I believe
> > some of the arguments are not as black and white as you point them out
> > at the moment and not all implications / choices have been discussed
> > (certainly not with me)  and documented. Maybe they are indeed obvious
> > and just not clear to me, but in this case maybe we can also use this
> > opportunity to better document the choices we took and the guarantees we
> > give.
> >
> > As you seem on vacation, there does not seem to be a huge urge to get
> > this change in (especially as the current status was there for almost
> > ten years). I will take the weekend to look into this again in great
> > detail, also taking into account the good arguments you give, the other
> > bug-reports that are affected, ...
> >
> > It would be nice if you could meanwhile revert the change. Thanks a lot.
> >
> > Best,
> > Tobias
> >
> > PS: I indeed know that you have one of the most solid compiler
> > backgrounds in the community, so your arguments weight strongly and I
> > indeed value your opinion very much.
> >
> > >
> > > On Thu, Mar 2, 2017 at 10:51 AM, Daniel Berlin <dberlin at dberlin.org>
> > > wrote:
> > >
> > > >
> > > >
> > > > On Tue, Feb 28, 2017 at 10:21 PM, Tobias Grosser <
> > > > tobias.grosser at inf.ethz.ch> wrote:
> > > >
> > > >> Hi Daniel,
> > > >>
> > > >> I unfortunately missed this code review and I really would have liked
> > to
> > > >> be part of it. First, I am very glad to see that you started to work
> > on
> > > >> this problem. It is one I thought about quite a bit. You might
> > remember
> > > >> earlier discussions on this review thread:
> > > >> https://reviews.llvm.org/D12676. I remember even at this time you
> > wanted
> > > >> to work on the (post) dom trees, but it took years for one of us (you
> > in
> > > >> this case) to move forward.
> > > >>
> > > >> Now, I am not sure the new behavior is entirely what I want.
> > > >
> > > >
> > > > I'm not sure i believe we should go by that :).
> > > >
> > > >
> > > >> In fact it
> > > >> goes contrary to what I proposed in the patch mentioned before.
> > > >
> > > >
> > > > Yes, it does, looking back.  I did not remember that at the time, my
> > > > apologies!
> > > >
> > > >
> > > >> For my
> > > >> use cases the completeness of the dominator tree (even though I can
> > > >> understand why people want it) is not as important as long as the
> > > >> dominator tree is not flattening out.
> > > >
> > > >
> > > > Correctness and completeness of the post-dom tree is, IMHO, a
> > base-level
> > > > requirement.
> > > >
> > > > Unfortunately, your patch flattens
> > > >> out the post dominator tree, which impacts RegionInfo and results in
> > > >> undetected regions (visible in the region-info test cases) which I
> > added
> > > >> very deliberately.
> > > >>
> > > >
> > > > Except they are not actually SESE regions, as i mentioned in each test
> > > > case.
> > > > SESE regions are not allowed to contain reverse-unreachable infinite
> > > > loops, in any canonical definition i can find.
> > > > You want SESE regions ignoring things that don't exit the program.
> > > > That's fine, but those are not canonical SESE regions, and if you want
> > to
> > > > form them, i don't think you have a reasonable argument that we should
> > have
> > > > llvm build a non-standard post-dominator tree for *everyone* to get
> > there.
> > > >
> > > >
> > > >>
> > > >> I need some more time to properly go through your patch. Would it be
> > > >> possible to revert it for now to give me a chance to review it myself?
> > > >>
> > > >
> > > >
> > > > I'm fine, but i'm going to be honest, i'm not up for arguing about this
> > > > again.
> > > > If someone wants to decide what we do here, i'd rather just see that
> > > > happen.
> > > > I do not believe we are going to agree, because my position is going to
> > > > be, strongly, that we should do what everyone else does, what every
> > > > compiler textbook teaches, and what everyone else expects.
> > > > Even though i've done so below, i really have no urge to argue the
> > > > theoretical basis of the post-dmo tree.
> > > > Really.
> > > > I just want something that actually does what everyone expects.
> > > > You want something different, and you have a valid use case for that.
> > But
> > > > that doesn't mean we should make that the standard.
> > > >
> > > >
> > > >> Until then, let me give you already a brief example of what I mean by
> > > >> "flattening out" based on your example in the commit message:
> > > >>
> > > >> On Tue, Feb 28, 2017, at 11:57 PM, Daniel Berlin via llvm-commits
> > wrote:
> > > >> > Author: dannyb
> > > >> > Date: Tue Feb 28 16:57:50 2017
> > > >> > New Revision: 296535
> > > >>
> > > >> [...[
> > > >>
> > > >> > declare void foo()
> > > >> > define internal void @f() {
> > > >> > entry:
> > > >> >   br i1 undef, label %bb35, label %bb3.i
> > > >> >
> > > >> > bb3.i:
> > > >> >   call void @foo()
> > > >> >   br label %bb3.i
> > > >> >
> > > >> > bb35.loopexit3:
> > > >> >   br label %bb35
> > > >> >
> > > >> > bb35:
> > > >> >   ret void
> > > >> > }
> > > >> > ```
> > > >> > We get:
> > > >> > ```
> > > >> > Inorder PostDominator Tree:
> > > >> >   [1]  <<exit node>> {0,7}
> > > >> >     [2] %bb35 {1,6}
> > > >> >       [3] %bb35.loopexit3 {2,3}
> > > >> >       [3] %entry {4,5}
> > > >> > ```
> > > >>
> > > >> We originally had information that bb35 post-dominates both
> > > >> %bb35.loopexit3  and %entry.
> > > >>
> > > >
> > > > Which it does not.  Really.
> > > >
> > > >
> > > >>
> > > >> The new post-dominator tree now is:
> > > >>
> > > >>   [1]  <<exit node>> {0,9}
> > > >>     [2] %bb35 {1,4}
> > > >>       [3] %bb35.loopexit3 {2,3}
> > > >>     [2] %entry {5,6}
> > > >>     [2] %bb3.i {7,8}
> > > >>
> > > >> Here we do not have the information that %bb35 post-dominates entry
> > any
> > > >> more
> > > >
> > > >
> > > > It 100% does not post-dominate entry.
> > > >
> > > > Every path to exit does not go through bb35.
> > > > You  seem to argue  this means "every real return", ignoring
> > non-returns.
> > > > I get why you want this for your case. I really do.
> > > > But i have ever seen a compiler implement this, and in fact, what we do
> > > > now, post-patch is the canonical thing to do, taught in compiler
> > textbooks,
> > > > and used by every other compiler.
> > > > It's also the canonical answer given on stackoverflow.
> > > > Ignoring non-returns causes bugs and brokenness (you can already see
> > one
> > > > hack i removed from adce to try to work around this, which, btw, would
> > fail
> > > > in certain cases).
> > > >
> > > >
> > > > If you need something different, my strong stance would be "implement
> > > > something different".
> > > > This is not because i'm trying to be difficult, but because i don't
> > think
> > > > we should be gratuitously different for the sake of a single use case
> > when
> > > > we have a lot of use cases on the other side (and in fact, the reason
> > i did
> > > > this patch is because it's blocking someone else's patch that requires
> > > > correct post-dominator behavior).
> > > > Additionally, we already know, as i pointed out in the review (linking
> > to
> > > > an entire blog post about how broken it is for people trying to use
> > it),
> > > > that a number of people directly disagree with your view of regions,
> > and
> > > > have built their own region frameworks, all which do the same thing
> > (the
> > > > thing we do now post-patch).
> > > > I can provide more if you like, with people complaining about our
> > > > implementation of post-dom.
> > > > On the other hand, I can find no example where people have done the
> > thing
> > > > you want done.  That, again, doesn't mean it's not valuable, just that
> > i
> > > > believe it's non-standard.
> > > > :)
> > > >
> > > >
> > > >
> > > >> (which is what I care about), but we have bb3.i in the
> > > >> post-dominator tree (which seems to be what you are aiming for).
> > > >>
> > > >> You say dropping the bb35 -> entry relationship is intentional:
> > > >>
> > > > Yes
> > > >
> > > >
> > > >>
> > > >> "  While it's true that it does not exit in a theoretical sense, it's
> > > >> not
> > > >>     really helpful to try to ignore the effect and pretend that bb35
> > > >>     post-dominates entry."
> > > >>
> > > >> AFAIU we agree on the theoretical definition that this relationship
> > > >> exists.
> > > >
> > > > I don't believe we really do.
> > > > We would agree that post-dominance is undefined in any program with
> > > > infinite loops. Literally.  That you have to pick some definition.
> > > > But there are lots of things that are undefined that people expect
> > > > standard behavior.
> > > > Past that, i would not agree that, in a program with infinite loops,
> > if we
> > > > defined it, that we would define it the way you would suggest.  I
> > strongly
> > > > believe we would define it the same way everyone else does, which is to
> > > > connect the backedge of the infinite loop to a virtual exit node. That
> > is
> > > > the canonical answer you will find everywhere, and there are good
> > > > reasonsfor this.
> > > >
> > > >> You also claim it is not helpful (which I disagree with).
> > > >>
> > > >
> > > > I understand that :)
> > > >
> > > >
> > > >>
> > > >> I am not sure if you are claiming that the existence of this
> > > >> relationship is actively hurtful. The following sentence might be
> > such a
> > > >> claim:
> > > >>
> > > >> "Worse, we pretend the infinite loop does nothing (it's usually
> > > >> considered a side-effect),"
> > > >>
> > > >> but I feel that this is more likely to refer to the fact that %bb3.i
> > is
> > > >> not part of the post-dom tree. Is this correctly interpreted?
> > > >>
> > > >
> > > > Partially. I also do not believe it is correct to say that bb35
> > > > post-dominates entry node.
> > > >
> > > >
> > > >> Just to get what you are aiming for. Would a post-dominator tree of
> > the
> > > >> form:
> > > >>
> > > >> [1]  <<exit node>>
> > > >>   [2] %bb35
> > > >>     [3] %bb35.loopexit3
> > > >>     [3] %entry
> > > >>       [4] %bb3.i
> > > >>
> > > >> be something you would be happy with?
> > > >
> > > >
> > > > No.
> > > > Because i do not believe  it is not correct in any arguable sense :)
> > > > It implies that bb35 post-dominates bb3.i, but it does not.
> > > > In fact, *zero* paths from bb3.i to program exit go through bb35,
> > > > Your argument seems to be  is "bb3.i doesn't exit so it doesn't
> > matter".
> > > > But the fact that it's not a child of the virtual exit implies such a
> > path
> > > > *does exist*.
> > > > Lots of algorithms, for correctness, depend on things being connected
> > to
> > > > the virtual exit in such a case (because, again, it's what everyone
> > does).
> > > > So i'm 100% the above representation will cause bugs.
> > > >
> > > > It also has the  disadvantage that you can't actually easily find the
> > > > blocks that exit the program or never exit, but that's secondary.
> > > >
> > > >
> > > >
> > > >> Did you consider forming such a
> > > >> tree and decide against it for a certain reason?
> > > >
> > > >
> > > > Yes, because it is IMHO not valid from a correctness perspective, and
> > this
> > > > will cause bugs.
> > > > Seriously. The existing code causes bugs in region formation and
> > elsewhere
> > > > already because of what we did.
> > > >
> > > >
> > > >> The intuition would be
> > > >> to connect the unreachable parts not to the exit node, but to the
> > basic
> > > >> block which connects the unreachable region with the reachable world.
> > > >>
> > > >
> > > > This will not generate correct information, as far as i know, and as
> > > > mentioned, a lot of algorithms depend on the loop being connected to
> > > > virtual exit.
> > > >
> > > >
> > > >>
> > > >> The reason I am interested in this is that we use regions to model
> > loop
> > > >> programs. The unreachable parts are not the ones interesting to me,
> > but
> > > >> I would like to know the branch conditions under which a program might
> > > >> run into an infinite loop (such that we can build assumptions that
> > this
> > > >> won't happen). To build such conditions, we currently use the region
> > > >> info.
> > > >>
> > > >
> > > > Again, my suggestion would be to build the tree you need.
> > > > I'm even happy to make it such a option for you and enable you to have
> > a
> > > > version of regioninfo that uses it.
> > > > It's not because i think what you are doing is not valuable, but
> > because i
> > > > think it is non-standard, and we've already run into enough bugs and
> > issues
> > > > with that non-standardness, for pretty much no benefit.
> > > > (as you can see, i already had to remove one hack in ADCE).
> > > >
> > > > So if you want to revert it because you want to argue we should do
> > > > something different *by default* (instead of optionally), i'm fine
> > with it,
> > > > i'm not sure how we come to consensus.
> > > > My position is going to be "no" and yours is going to be "yes". :)
> > > >
> > > >
> > > >
> >
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits