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

Thu Mar 2 10:51:43 PST 2017

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/28a7bfb5/attachment.html>