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

Tobias Grosser via llvm-commits llvm-commits at lists.llvm.org
Thu Mar 2 12:10:11 PST 2017


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". :)
> >
> >
> >


More information about the llvm-commits mailing list