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

[Daniel Berlin via llvm-commits]

On Wed, Jan 25, 2017 at 6:40 PM, Serge Pavlov <sepavloff at gmail.com> wrote:

> 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.
>
>
FWIW, I am not an expert in the pass manager, but that makes no sense.


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

This strategy of "no point to run, but no result available", seems ...
concerning.
But again, i'll leave the viewpoints on the passmanager to chandler.


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

We're going to agree to disagree here.


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

Then the structure should not have been created. Period.


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

Again, i'll leave the pass manager strategy to chandler, but this whole
thing smacks me as wrong in a number of ways.
If it's really that this is a one-off, okay, but otherwise i suspecft we
will run into this problem again with something else.


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

If Chandler truly believes that is not the right way forward, i'd really
love to hear his opinion on what should be happening here.
Assuming he's fine with this one off, i think your #1 below is okay.


>
> `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/20170130/68c8ae8e/attachment.html>