[PATCH] D12676: Ensure a complete post-dominance tree is built in the presence of unreachables

[Daniel Berlin via llvm-commits]

(The other part will require some thinking)


>> Note that in general, it's not going to be possible to determine
>> reachability, and what we should exclude anyway, without a DFS walk
>
>
> I think these are two issues: I am in this patch mainly concerned about the
> post-dominator tree being correct for dominance relations that are caused by
> a path
> that goes through two basic blocks and ends at an exit of the function. This
> is what is well defined and for this I do not see why another DFS walk would
> be needed.
> The set of exits is well defined and can just be added, no?

As long as they end in either an explicit unreachable, or the exit
block, yes it is.

> What you seem to aim for below is to define a relation similar to
> post-dominance for blocks that are not on any path that
> finishes in an exit block of the function.
This was the initial goal of the check it has, yes.

>  To my understanding, this would
> correspond to adding 'otherbb' somehow into the dominator tree (e.g. to
> establish a dominance-like relation in parts of the tree that can not be
> reached from the exit node. I believe this is considerably more involved and
> indeed
> requires some larger restructuring.

This is, AFAIK, the world that is supposed to exist now :)
It is, AFAIK, an explicit goal.
I actually have no desire for this goal personally. Others did.  I am
simply trying to fulfill the requirement that folks want unreachable
nodes to give some answer for dominance and post-dominance.
(I personally think giving dominance answers about unreachable nodes
is ill-defined and a bad idea :P)

> Do you have use cases were having such
> relation actually is beneficial in some way? I thought about this myself,
> but did
> not yet find an example where this would be useful.

We expect queries on unreachable nodes to work :)

If we don't want to make it work, great, i have no problem with this.
Chandler did, IIRC.
In that case, if we visit nothing but have blocks, we should add a
virtual root node and call it a day.

>>
>> Given that we know that the only way (at least, i can think of) to avoid
>> adding another DFS walk is to make the root finding part of dom tree
>> construction, i'd rather not add see us add isDomExit, because it's another
>> thing that will need to be cleaned up to make this happen.
>> (The bug has more details).
>
>
> Given my explanation above, I think this patch makes sense. However, let's
> see first if we can reach a common understanding on the issues.
>
>> ================
>> Comment at: include/llvm/IR/Dominators.h:119
>> @@ +118,3 @@
>> +      GraphTraits<Function *>::child_end(N))
>> +    return false;
>> +
>> ----------------
>> This is not a sufficient check for non-reachability.
>>
>> What if i have a infinite self-loop?
>>
>> https://llvm.org/bugs/show_bug.cgi?id=24415
>>
>> (extend it to multiple blocks if you like)
>
>
> This is not intended as check for non-reachability. This is intended as a
> check of where to start the DFS search that determines reachability.

Except, in that sense, it's wrong, because it needs to include all
nodes that are reverse-unreachable, unless they end in unreachable
instructions.

At least, that is it's goal: to find all reverse unreachable blocks to
use them as children of the fake exit block.
As i've shown, it misses plenty of these cases.

Otherwise, what is the set of blocks it is trying to find, exactly?

IE why choose "some blocks that are reverse-unreachable, but not others"

(This is somewhat rhetorical, you can look up the history of adding
this, and see this is what it was intended to handle :P)

>
> Best,
> Tobias