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

[Daniel Berlin via llvm-commits]

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.

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". :)
> > >
> > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170302/664f1a30/attachment.html>