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

[Daniel Berlin via llvm-commits]

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/20170314/88ad90c6/attachment.html>