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

[Tobias Grosser via llvm-commits]

On 09/07/2015 10:01 PM, Daniel Berlin wrote:
> (The other part will require some thinking)

Wow, you are quick.

>>> 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.

Good.

Besides, that explicit unreachables, the way they are added today weaken
the post-dominance tree. I believe adding them, even though they are not
exits, is wrong in the absence of any special handling (second CFG run).
  
>> 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.

Interesting. Judging from the lack of test cases for unreachable and the
absence of any comments documenting how they are handled, my feeling is
that the current handling may not have been fully intentional.
  
>>   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.

Interesting. Is this documented somewhere? At the moment, it seems to mostly
weaken the post-dominance tree for the path that is actually reachable.

> 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)

OK, then we share the same feelings. Can you point me to the document/email-thread
where people have asked for this feature? Test cases that they would like
to have working, ...?
  
>> 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 :)he

Does this mean you also intend to "fix" the case of infinite  loops?  It will
be interesting to see how/if you want to define a post-dominance relation
there, without weakening the post-dominance tree we currently compute
in a similar way it is weakened by our current handling of unreachables.

> 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.

I currently have no need for unreachables/infinite loops, but if we can
add them without weakening the common case, that seems fine as well.
I definitely would be interested in the motivation and the approach
that will be taken.

>>> Given that we know that the onlay 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.

Can you help me out what reverse-unreachable means?
  
> 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)

Right, the current code is somehow inconsistent. It handles some
unreachable instructions in a bad way and others not at all.

This patch makes this check consistent. We now only look at the exit
nodes and get the correct and well-defined  post-dominance tree for
the reachable part of the CFG. This seems to be an improvement over
what we have now.

I will leave other people some time to jump in, but my feeling is
that we better should get the core right (without loosing
precision) and then discuss the implications of a well-defined
extension to unreachable/infinite loops.

Best,
Tobias