<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 14, 2017 at 4:43 PM, Michael Kruse <span dir="ltr"><<a href="mailto:llvm-commits@meinersbur.de" target="_blank">llvm-commits@meinersbur.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Hi,<br>
<br>
just to add another point of view.<br>
<br>
<br>
An intuitive definition of postdominator of B could be "a block that<br>
will eventually be executed when B is executed".<br>
<br>
A formal definition is "forall execution paths to the exit lays B".<br>
<br>
These definitions do not match. First, on the path to B there might be<br>
a loop whose exit condition is never true. Due to Turing's<br>
Halteproblem, we actually can never now whether B will be executed<br>
eventually. Second, if there is no path to the exit at all, the formal<br>
definition effectively says that every block is a postdominator (<br>
vioforall over empty set => true). That's obviously not useful in<br>
practice. One can just say that postdominance is undefined in that<br>
case, as the current PostDominatorTree does.<br>
<br>
Michael Wolfe's book suggests to add "phantom edges" to connect the<br>
infinite loop to the rest of the tree.</blockquote><div><br></div><div>This is precisely what both patches do.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> I will use the same term. It's<br>
an edge that is never executed. Indeed, one could have rewritten the<br>
IR from<br>
<br>
    loop:<br>
      br label %loop<br>
<br>
to<br>
<br>
    loop:<br>
      br i1 false, label %somewhere, label %loop<br>
<br>
without changing semantics, </blockquote><div><br></div><div>That's only because then that path *actually* exists in the CFG.</div><div>There is actually successor and predecessor edges in the CFG.</div><div>It's not a fake path</div><div>When the path exists in the CFG, it's a valid post-dom tree..</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">the same way as PostDominatorTree interprets<br>
<br>
    return1:<br>
      ret void<br>
<br>
    return2:<br>
      ret void<br>
<br>
as<br>
<br>
    return1:<br>
      br label %virtual_sink<br>
<br>
    return2:<br>
      br label %virtual_sink<br>
<br>
    virtual_sink:<br>
      ret void<br>
<br></blockquote><div>While this, nothing will remove ;)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I haven't seen arguments against that.<br></blockquote><div><br>No, because it's a straw-man. if you add the path to the cfg, ... then it exists in the cfg.</div><div>:)</div><div>We are talking about the case where a path does not exist in the CFG, but does exist according to the post-dom tree properties.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
The path invariant "if B is a postdominator, then there is a path to<br>
B" only derives from the intuitive definition. It's what programmers<br>
would typically expect, and might be a goal to adhere in order to<br>
avoid bugs. As already said, programmers cannot expect then B is<br>
eventually executed.<br></blockquote><div><br></div><div><br></div><div>Sorry, but no, actually.</div><div>It's not about actual execution or non-execution. Its about where there are paths to.</div><div>This is a base level invariant of the dominator tree, not "a property people are used to".</div><div>Without this property, the dominator tree does not function as a dominator tree. The literature i pointed to you actually goes through a formal proof of this.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
However, even D29705 violates this. It adds a path to the virtual exit<br>
even though there is no path to the exit.</blockquote><div><br>Actually, it does not violate this.</div><div>You yourself just said why.</div><div>It does not claim there is a path to exit, it claims there is a path to "a fake exit".</div><div>No actual exit block in the program is incorrect.</div><div><br></div><div>That is, the only thing it claims it has a path to is an inaccessible node we created, and is not really accessible in LLVM.</div><div>All paths through the CFG you can walk in the program have proper and corresponding correct paths in the post-dominator tree.</div><div>Every path the post-dominator tree invariants say exists actually exists in the underlying CFG, if you treat the virtual exit node as just "an entry point to the forest".</div><div><br></div><div>You'll also note that because all reverse-unreachable paths are directly connected to the virtual exit, you can also ignore the virtual exit except as a thing that gives you children, and it will give correct answers.</div><div><br></div><div>It is just a root node for our forest.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> The only difference in<br>
D30890 is the node it connects to.</blockquote><div><br></div><div>Again, this is not correct either.</div><div>As opposed to the above, the paths through the CFG *no longer* match the paths through the post-dominator tree.  The post-dominator tree invairants say paths exist that do *not* exist in the actual CFG.</div><div>That is in fact, the invariant it breaks, and again, as mentioned, this invariant one of the only things that makes a dominator tree a dominator tree.</div><div><br></div><div>If you added actual cfg edges, *and kept them*, then the produced post-dominator tree would be correct, and optimizations/etc would perform correctly.</div><div>But you aren't adding those CFG edges.</div><div>So it's not.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I don't think one is more "correct"<br>
than the other. </blockquote><div><br></div><div>I *very very very* strongly disagree, because, as i've said repeatedly, and pointed to literature proving, it breaks the very base level invariants that make the dominator tree the dominator tree.</div><div>See, again, <a href="https://arxiv.org/pdf/1604.02711.pdf">https://arxiv.org/pdf/1604.02711.pdf</a>, lemma 2.1, and theorem 2.2</div><div><br></div><div>"Tree T is the dominator tree (T = D) if and only if it has the parent and the
sibling properties."</div><div><br></div><div>The D30890 dominator tree does *not* have the parent property with respect to the CFG.</div><div>The D29705 does.</div><div><br></div><div>See also <a href="http://dl.acm.org/citation.cfm?id=2764913">http://dl.acm.org/citation.cfm?id=2764913</a> an entire paper dealing with "<span style="color:rgb(0,0,0);font-family:verdana,arial,helvetica,sans-serif;font-size:12.8px">How does one verify that the dominator tree of a flow graph is correct?</span><span style="color:rgb(0,0,0);font-family:verdana,arial,helvetica,sans-serif;font-size:12.8px"> "</span></div><div>You'll note the post-dominator tree generated by D30890 is not certifiable. You can't verify the correctness because it is not actually correct with respect to the flow graph unless you *actually* add the edges you are talking about *and keep them there*.</div><div>In contrast, the post-dominator tree generated by D29705 *is* certifiable, </div><div>Again, i'm strictly staying with "correct", not "useful". I am happy to believe there are passes that can use less the a fully-correct tree, regioninfo appears to be one of them.</div><div><br></div><div>The next question i get is "well, what does that really break", and the answer is "really, everything, in very subtle ways, and some things in less subtle ways".</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">There is no change in semantics by phantom edges.<br></blockquote><div>Again i disagree when those edges need to be in the CFG but are not.</div><div>The virtual exit node is not a CFG node, so adding/removing edges to it does not break any property of the dominator tree with respect to the CFG.</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Again, there is no guarantee that an edge is ever taken. Passes that<br>
are only correct without phantom edges would also be wrong if there<br>
was a real edge.<br></blockquote><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Daniel already mentioned that PostDomTree currently does actually<br>
connect dead-end blocks to the virtual exit node, done by this code in<br>
GenericDomTree.<br>
<br>
      for (auto *Node : nodes(&F))<br>
<span class="gmail-">        if (TraitsTy::child_begin(Node) == TraitsTy::child_end(Node))<br>
</span>          addRoot(Node);<br>
<br>
This fails to detect infinite loops, so it is reasonable to see this<br>
just as a bug in the current implementation. </blockquote><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
An issue I can think of is that it is underdefined which block of an<br>
infinite loop gets connected, but significantly determines the postdom<br>
tree. We could just connect all of them. If we introduce another<br>
virtual sink node for non-exiting exists, all blocks of the loop<br>
become direct children of that node, such that we "flattened" all the<br>
structure in it. Let's call that virtual sink node "nullptr" and we<br>
something similar as we have now.<br>
<br>
Eli Friedman already mentioned the consistency between different<br>
analyses in D30890, i.e. different analyses have different views of<br>
the same CFG. I think he's right that consistency is harder to<br>
maintain if phantom edges go into the middle of a CFG instead to the<br>
PostDomTree-only virtual node. The first even creates virtual loops,<br>
the latter doesn't.<br>
<br>
Let's go into detail how PostDom is used in LLVM passes.<br>
<br>
<br>
RegionInfo<br>
<br>
Tobias has a concrete vision of what an SESE region is. Phantom edges<br>
to the exit destroy the single exit property and hence don't form such<br>
a region anymore. A workaround should be possible if there is a<br>
distinct virtual sink for infinite loops. RegionInfo only needs to<br>
take into account that that sink can be member of multiple non-nested<br>
regions.<br>
<br>
I personally think that LLVM's RegionInfo is something particular in<br>
LLVM. I'd prefer something that better interacts with LoopInfo.<br>
Currently Regions and Loops can intersect in any possible way. The<br>
Dragonbook defines regions through loops and their bodies. This means<br>
SEME regions.<br>
<br>
<br>
Aggressive Dead Code Elimination<br>
<br>
ADCE uses IDFCalculator to determine control dependencies. It<br>
implicitly assumes that only blocks reachable from the PostDomTree's<br>
virtual root are live. That's obviously not true, so it just marks all<br>
block not in the PostDomTree as live (Strangely, even twice).</blockquote><div><br></div><div>The twice is because it's broken due to our current breaking the path invariant, which breaks IDF calculator in the precise way i mentioned previously.</div><div>It's trying to hack around this.</div><div><br></div><div>I'm sure you can produce cases where it gets wrong answers as a result, as these hacks cannot work in all cases.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
SSUPRE<br>
<br>
Single Status Use-Form Partial Redundancy Elimination? This is not<br>
implemented in LLVM, and I cannot say how it could be influenced.<br></blockquote><div><br></div><div>The pass was posted for review recently, as PDSE.</div><div>It relies on the post-dominator tree parent property as well.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
<br>
In summary, I tried not to portrait one variant to be better than the<br>
other. But I am convinced that there is not only one sane way to do<br>
it, both ways seem viable.<br></blockquote><div><br></div><div>Again, i'm going to very strongly disagree.</div><div>and at this point, i'm going to stop arguing, as i promised Tobias i would when we started.</div><div>It's clear to me we won't reach consensus.</div><div><br></div><div>If we really wish to break the invariants that make the post-dominator tree the post-dominator tree, and decide we know better than literally every other compiler to confront this issue, well then, okay.</div><div>It makes no sense to me do that, and what you get is not actually a post-dominator tree.</div><div>it's "a post-dominator tree for reachable nodes, a random tree we kinda like for reverse-unreachable nodes".</div><div><br></div><div>But i'm not going to participate in that, and i'm not going to try to write optimizations for it.</div><div>Because i know it won't work.</div><div>It already doesn't work.</div><div><div>Like i said, no mechanism you can use to formally certify correctness of dominator trees will even say it is correct either.</div></div><div><br></div><div>That's not a threat, it's more "i don't have the energy to fight this fight anymore". </div><div>So y'all do what you want at this point.</div><div><br></div></div></div></div>