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

Wed Jan 25 18:40:19 PST 2017

2017-01-26 0:41 GMT+07:00 Daniel Berlin <dberlin at dberlin.org>:

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

Agree. But it is just the case we have, - the analysis was run, no errors
found, but it cannot return DominatorTree because it didn't calculate the
latter.

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

This is the root of the problem. We have an analysis that was run without
errors but it cannot return its result, it decided that there is no point
to run. It is a limitation of the Pass Manager, the latter assumes that if
a pass has started, it will return its result or fail. There is no option
"result hasn't calculated but it is OK". The patch in
https://reviews.llvm.org/D27190 tried to introduce additional state flag to
the `Pass` structure to detect if a pass run successfully but didn't
calculated results. With it `getAnalysisIfAvailable` could be used to get
pass results. Without such info, we have to deduce if the pass produced its
result by indirect evidence. Luckily we have now only one problematic pass.

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

It cannot violates invariants of something that does not exist :)
 DominatorTree was not calculated in this case.

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

Wrong premise: the dominator tree **was not** calculated.
Also the invalid dominator tree is not used by other passes, as they are
skipped. But the Pass Manager tries to use it by making call to
`verifyDomInfo` in `verifyPreservedAnalysis` and it leads to crash.

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

Yes, but nobody tries to use the invalid domtree because all potential
users are skipped. It is a problem of pass verification which runs only if
expensive checks are enabled. The pass manager doesn't know that the pass
was skipped and tries to get its result.

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

This way exactly was implemented in D27190. It was recognized as too
complex.

You are trying to use a datafield in the invalid data structure to try to
> signal that.
>

I don't use the data structure, it is just a check if the datastructure is
valid, similar check if a pointer is null.

That is  wrong, since nothing *else* will know it's invalid.
> Only verifyDomTree will know.
>

Nothing else needs it because everything is skipped. Only Pass Manager
suffers.

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

We don't use it, only check if it is valid.

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

This is a kind of hack, I agree. If it is not acceptable, we need to
resurrect D27190.

`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"
>

Yes, the statement "users don't work with it directly" is wrong. "Users" is
also wrong, is this case there is only one user that calls `verifyDomTree',
it is the Pass Manager. The idea was that the user should not aware how
`verifyDomTree` detects if the pass indeed has produced results.

To move forward I see several directions:
1. change the implementation of `MachineDominatorTree` so that it uses null
pointer if dominator tree is unavailable,
2. return to the solution in D27190 and keep flag 'results available' is
the Pass state,
3. something else?

Do you think case 1 is OK?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170126/57f4fb31/attachment.html>