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

[Daniel Berlin via llvm-commits]

See also, btw,
https://arxiv.org/abs/1608.06462
literally the first sentence of the abstract.

A flow graph G=(V,E,s) is a directed graph with a distinguished start
vertex s. The dominator tree D of G is a tree rooted at s, such that a
vertex v is an ancestor of a vertex w if and only if all paths from s to w
include v.

Thus, it is legal, for example, to do the following with a dominator or
post-dominator tree:

for each dominator tree node:
  Walk idoms back to virtual root (ignoring virtual root), put them in a
set.
  Get the associated CFG node A
  for each thing in set:
     perform depth_first_search starting at A in forward or backwards
direction (for dom/post-dom)
     assert that we find idom.

This will pass, with no issue, on D29705
This will miserably fail on D30890, because the cfg paths don't actually
exist.

This breaks literally every algorithm that relies on the post-dominator
tree for it's path properties (to speed up propagation order, etc), as, for
example, the linear time IDF calculator does.

It probably does *not* break those relying on it as a measure of control
dependence, because it's only an approximation to control dependence
anyway, as has been discussed ad-nauseum.




On Tue, Mar 14, 2017 at 5:57 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

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