<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 28, 2017 at 10:21 PM, Tobias Grosser <span dir="ltr"><<a href="mailto:tobias.grosser@inf.ethz.ch" target="_blank">tobias.grosser@inf.ethz.ch</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Daniel,<br>
<br>
I unfortunately missed this code review and I really would have liked to<br>
be part of it. First, I am very glad to see that you started to work on<br>
this problem. It is one I thought about quite a bit. You might remember<br>
earlier discussions on this review thread:<br>
<a href="https://reviews.llvm.org/D12676" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D12676</a>. I remember even at this time you wanted<br>
to work on the (post) dom trees, but it took years for one of us (you in<br>
this case) to move forward.<br>
<br>
Now, I am not sure the new behavior is entirely what I want.</blockquote><div><br></div><div>I'm not sure i believe we should go by that :).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> In fact it<br>
goes contrary to what I proposed in the patch mentioned before.</blockquote><div><br></div><div>Yes, it does, looking back. I did not remember that at the time, my apologies!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> For my<br>
use cases the completeness of the dominator tree (even though I can<br>
understand why people want it) is not as important as long as the<br>
dominator tree is not flattening out. </blockquote><div><br></div><div>Correctness and completeness of the post-dom tree is, IMHO, a base-level requirement.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Unfortunately, your patch flattens<br>
out the post dominator tree, which impacts RegionInfo and results in<br>
undetected regions (visible in the region-info test cases) which I added<br>
very deliberately.<br></blockquote><div><br></div><div>Except they are not actually SESE regions, as i mentioned in each test case.</div><div>SESE regions are not allowed to contain reverse-unreachable infinite loops, in any canonical definition i can find.</div><div>You want SESE regions ignoring things that don't exit the program.</div><div>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.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I need some more time to properly go through your patch. Would it be<br>
possible to revert it for now to give me a chance to review it myself?<br></blockquote><div><br></div><div><br></div><div>I'm fine, but i'm going to be honest, i'm not up for arguing about this again.</div><div>If someone wants to decide what we do here, i'd rather just see that happen.</div><div>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.</div><div>Even though i've done so below, i really have no urge to argue the theoretical basis of the post-dmo tree.</div><div>Really.</div><div>I just want something that actually does what everyone expects.</div><div>You want something different, and you have a valid use case for that. But that doesn't mean we should make that the standard.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Until then, let me give you already a brief example of what I mean by<br>
"flattening out" based on your example in the commit message:<br>
<span class=""><br>
On Tue, Feb 28, 2017, at 11:57 PM, Daniel Berlin via llvm-commits wrote:<br>
> Author: dannyb<br>
> Date: Tue Feb 28 16:57:50 2017<br>
> New Revision: 296535<br>
<br>
</span>[...[<br>
<span class=""><br>
> declare void foo()<br>
> define internal void @f() {<br>
> entry:<br>
> br i1 undef, label %bb35, label %bb3.i<br>
><br>
> bb3.i:<br>
> call void @foo()<br>
> br label %bb3.i<br>
><br>
> bb35.loopexit3:<br>
> br label %bb35<br>
><br>
> bb35:<br>
> ret void<br>
> }<br>
> ```<br>
> We get:<br>
> ```<br>
> Inorder PostDominator Tree:<br>
> [1] <<exit node>> {0,7}<br>
> [2] %bb35 {1,6}<br>
> [3] %bb35.loopexit3 {2,3}<br>
> [3] %entry {4,5}<br>
> ```<br>
<br>
</span>We originally had information that bb35 post-dominates both<br>
%bb35.loopexit3 and %entry.<br></blockquote><div><br></div><div>Which it does not. Really.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The new post-dominator tree now is:<br>
<br>
[1] <<exit node>> {0,9}<br>
[2] %bb35 {1,4}<br>
[3] %bb35.loopexit3 {2,3}<br>
[2] %entry {5,6}<br>
[2] %bb3.i {7,8}<br>
<br>
Here we do not have the information that %bb35 post-dominates entry any<br>
more</blockquote><div><br></div><div>It 100% does not post-dominate entry.</div><div><br></div><div>Every path to exit does not go through bb35.</div><div>You seem to argue this means "every real return", ignoring non-returns. I get why you want this for your case. I really do.</div><div>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. </div><div>It's also the canonical answer given on stackoverflow.</div><div>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).</div><div><br></div><div><br></div><div>If you need something different, my strong stance would be "implement something different".</div><div>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).</div><div>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). </div><div>I can provide more if you like, with people complaining about our implementation of post-dom.</div><div>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.</div><div>:)</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> (which is what I care about), but we have bb3.i in the<br>
post-dominator tree (which seems to be what you are aiming for).<br>
<br>
You say dropping the bb35 -> entry relationship is intentional:<br></blockquote><div>Yes</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
" While it's true that it does not exit in a theoretical sense, it's<br>
not<br>
really helpful to try to ignore the effect and pretend that bb35<br>
post-dominates entry."<br>
<br>
</span>AFAIU we agree on the theoretical definition that this relationship<br>
exists. </blockquote><div>I don't believe we really do.</div><div>We would agree that post-dominance is undefined in any program with infinite loops. Literally. That you have to pick some definition.</div><div>But there are lots of things that are undefined that people expect standard behavior.</div><div>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.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">You also claim it is not helpful (which I disagree with).<br></blockquote><div><br></div><div>I understand that :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I am not sure if you are claiming that the existence of this<br>
relationship is actively hurtful. The following sentence might be such a<br>
claim:<br>
<span class=""><br>
"Worse, we pretend the infinite loop does nothing (it's usually<br>
considered a side-effect),"<br>
<br>
</span>but I feel that this is more likely to refer to the fact that %bb3.i is<br>
not part of the post-dom tree. Is this correctly interpreted?<br></blockquote><div><br></div><div>Partially. I also do not believe it is correct to say that bb35 post-dominates entry node.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Just to get what you are aiming for. Would a post-dominator tree of the<br>
form:<br>
<br>
[1] <<exit node>><br>
[2] %bb35<br>
[3] %bb35.loopexit3<br>
[3] %entry<br>
[4] %bb3.i<br>
<br>
be something you would be happy with? </blockquote><div><br></div><div>No.</div><div>Because i do not believe it is not correct in any arguable sense :)</div><div>It implies that bb35 post-dominates bb3.i, but it does not.</div><div>In fact, *zero* paths from bb3.i to program exit go through bb35, </div><div>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*.</div><div>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).</div><div>So i'm 100% the above representation will cause bugs.</div><div><br></div><div>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.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Did you consider forming such a<br>
tree and decide against it for a certain reason?</blockquote><div><br></div><div>Yes, because it is IMHO not valid from a correctness perspective, and this will cause bugs.</div><div>Seriously. The existing code causes bugs in region formation and elsewhere already because of what we did.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> The intuition would be<br>
to connect the unreachable parts not to the exit node, but to the basic<br>
block which connects the unreachable region with the reachable world.<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The reason I am interested in this is that we use regions to model loop<br>
programs. The unreachable parts are not the ones interesting to me, but<br>
I would like to know the branch conditions under which a program might<br>
run into an infinite loop (such that we can build assumptions that this<br>
won't happen). To build such conditions, we currently use the region<br>
info.<br></blockquote><div><br></div><div>Again, my suggestion would be to build the tree you need.</div><div>I'm even happy to make it such a option for you and enable you to have a version of regioninfo that uses it.</div><div>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.</div><div>(as you can see, i already had to remove one hack in ADCE).</div><div><br></div><div>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.</div><div>My position is going to be "no" and yours is going to be "yes". :)</div><div><br></div><div><br></div></div></div></div>