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

Thu Mar 2 10:54:33 PST 2017

(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)

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/1fba46d8/attachment.html>