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