[PATCH] D28767: Do not verify Dominator tree if it has no roots

[Daniel Berlin via llvm-commits]

On Wed, Jan 25, 2017 at 9:16 AM, Serge Pavlov <sepavloff at gmail.com> wrote:

> 2017-01-25 23:20 GMT+07:00 Daniel Berlin <dberlin at dberlin.org>:
>
>> +Chandler.
>>
>>
>> On Wed, Jan 25, 2017 at 7:04 AM, Serge Pavlov <sepavloff at gmail.com>
>> wrote:
>>
>>> The fact is that sometimes domtree is not available.
>>>
>>
>>
>> Then why is there a DominatorTree * to use?
>> It's fine for the analysis to be not available. When that happens,it
>> should not produce a DominatorTree *.
>>
>>
> `DominatorTree` is not an anaysis.
>

You are right, DominatorTreeAnalysis or whatever is.

If we run that analysis, it must return a DominatorTree. That DominatorTree
better be valid.
Otherwise, we done messed up :)

>
>
`MachineDominatorTree` is an analysis an it is available,
`getAnalysis<MachineDominatorTree>()`
> returns not null.
>

Right.
If that happens, the dominator tree better be valid.
If it's not going to be valid, we need a good way to signal that in these
analysis (currently, both of them use either direct objects or references,
sadly) failed, and it needs to be a way that everything can understand, not
just "verifyDomTree".




>
>
>> IE DominatorTree * should be a nullptr you can check against.
>>
>>
>
> It can be, not should. `DT` is a private field of `MachineDominatorTree`,
> there are no requirements how to represent DominatorTree and how to work
> with it inside the pass. Using the check for presence of roots is merely
> simpler, the patch is more compact.
>

It's definitely simpler, it just violates all the invariants of the
DominatorTree structure :)
We should avoid that, where, here, we can do that.


>
>
>> If a function is `available_externally`  all codegen passes are skipped,
>>> although they are marked as required.
>>>
>>
>> See above :)
>>
>>
>>> MachineDominatorTree is also skipped and there is no domtree at all.
>>>
>>
>> Again, so why is there a DominatorTree * to use?
>>
>
> There are no problems to change implementation of `MachineDominatorTree`
> to use null pointer as indicator of unavailable domtree. It however doesn't
> present any gain, just another way to fetch info that the pass results are
> not available.
>
>
>>
>>> Attempt to call `getRoot` for such domtree will obviously cause a crash.
>>> There is no way to get 'right' domtree in this case, so prior to using it
>>> we must either:
>>> 1. Check if corresponding pass was indeed run, or
>>> 2. Check the domtree state trying to reveal if it is valid.
>>> The first approach was taken in https://reviews.llvm.org/D27190 but it
>>> was not accepted. This fix tries to use variant 2.
>>>
>>
>>
>>>
>>> Just to emphasize a key point: there are cases when domtree is absent
>>> because the pass that it creates was not run and this behavior is by design.
>>>
>>>
>> and in those cases, we should  not try to create an invalid
>> datastructure. We should create no datastructure, and let people test
>> against the nullptr.
>>
>> We do not create invalid datastructures,
>

I'm not sure how you can argue this.
The dominator tree you have constructed is not valid.  You are then using
the invalid datastructure in a different analysis, which happens to know to
error out that it's not valid.

What if i took the result of the analysis, and instead of calling "verify"
on it, i called getRoot().

It'll assert, because it's not a valid datastructure.

So either we shouldn't have constructed it that way, or we need a way to
signal "i ran this thing but it didn't return something valid".
You are trying to use a datafield in the invalid data structure to try to
signal that.
That is  wrong, since nothing *else* will know it's invalid.
Only verifyDomTree will know.




> we use particular state of a data field to deduce that the pass does not
> have results, `getAnalysis` is useless in this particular case.
>
> You should not use the state of a data field in a data structure that has
invariants you are violating :).
You should not embed that knowledge that "hey, if this is like this, what
it really means is X" in a single function in that datastructure.

If we really need a way to signal that the analysis ran, but the result is
invalid, we should build that, not hack it up.


> `MachineDominatorTree` is a pass and must obey API conventions,
> `getAnalysis` may be called for it and if it returns nullptr, a user knows
> that results are absent. `DominatorTree` is an internal datastructure,
> users don't work with it directly,
>

This is 100% false.
DominatorTree is a datastructure used *all over the place* directly.

Users do everything from touching the nodes (at least 300 places) to
sorting the children vector to whatever.

Now, maybe the machine level is different, but even there, this seems quite
wrong as a solution to "how do i signal that the analysis result is invalid"
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170125/3cd24ee3/attachment.html>