[PATCH] D21464: [PM] WIP: Introduce basic update capabilities to the new PM's CGSCC pass manager, including both plumbing and logic to handle function pass updates.

[Xinliang David Li via llvm-commits]

On Fri, Jul 8, 2016 at 3:11 AM, Daniel Berlin <dberlin at dberlin.org> wrote:

>
>
> On Fri, Jul 8, 2016 at 5:14 PM, Xinliang David Li <davidxl at google.com>
> wrote:
>
>>
>>
>> On Thu, Jul 7, 2016 at 5:03 PM, Daniel Berlin <dberlin at dberlin.org>
>> wrote:
>>
>>>
>>>
>>> On Fri, Jul 8, 2016 at 8:31 AM, Chandler Carruth <chandlerc at google.com>
>>> wrote:
>>>
>>>> On Thu, Jun 30, 2016 at 6:36 PM Daniel Berlin via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>>
>>>>>> 4. The patch introduces a new SCC formation algorithm (double layer
>>>>>> with support for CG mutation on the fly), however design document on how
>>>>>> this works is missing.  It needs to document
>>>>>>     * What exactly is expected (in terms of vistation order) when a)
>>>>>> an edge is added; b) an edge is deleted; c) when a new node is introduced
>>>>>> and connected.
>>>>>>     * How ref-edges are formed, how indirect callsites are handled
>>>>>> etc.
>>>>>>     * If it is a modification of the classic SCC formation algorithm,
>>>>>> describe the change and prove that the algorithm works as expected.
>>>>>>
>>>>>>
>>>>> FWIW: I agree that for algorithms this complex, where we have no other
>>>>> reference for how they are supposed to work, and no way to know what
>>>>> invariants make them correct or not (other than reading code), we should
>>>>> have some design doc.
>>>>>
>>>>
>>>> Please see my response to David, there is no disagreement here.
>>>>
>>>>
>>>>>
>>>>> In this case, it should not be hard to prove that you can validly form
>>>>> both ref and non-ref SCC's at once:
>>>>>
>>>>
>>>> I'd really like to move this to a separate discussion of LCG. Maybe in
>>>> response to a separate patch that adds the needed documentation.
>>>>
>>>> I'm fine if that has to conclude before we review this patch, but this
>>>> patch is pretty much exclusively using the LCG API to do updates.
>>>>
>>>
>>> That's fine.
>>>
>>> I stared at the algorithm it uses, and it is as i expected (and contrary
>>> to what i think david said).  It looks like it uses tarjan to revisit the
>>> nodes in a found refscc in order to figure out the contained SCC's, which
>>> should be provably correct.  It is not strictly necessary to do this, but
>>> it's provably O(2N) instead of O(N) worst case, so who cares unless it
>>> turns out to be a performance problem in practice, who cares.  The only
>>> case i could see it mattering is if you have large ref sccs where the
>>> internal sccs are all single node or something.
>>>
>>
>> Wait -- what you described here is what I said too,
>>
>
> Well, not quite. Happy to go into the differences :P
>
>> but the algorithm is different from your proposal (which is more
>> efficient) :)
>>
>> Yes, it is. :P
>
>
>> For reference,  I am not concerned about this ref-scc/scc construction
>> part, but the part that deals with update.  I believe Chandler himself have
>> the same concerns according to what he wrote in the summary (bullet 2, 3
>> etc) for the patch.
>>
>
>
>
>>
>>
>>>
>>> It still should be overall documented (as we've all agreed), so that me
>>> and you are hopefully not the only two who understand it :)
>>>
>>
>>
>>>
>>> The update algorithms could be sped up in time bounds, but again,
>>> without data saying it's a perf problem, i'd pass on it since it looks like
>>> they have verification functions to make sure they work.
>>>
>>
>> I think it is mostly the update algorithm that needs to be
>> documented/examined, which is also strongly tied to the rationale why
>> refSCC is needed. If it is the core part of the new CGSCC PM, we need to
>> make sure the path taken is fully correct.
>>
>
> So, i actually went to go see if anyone had proved a tight bound on update
> times yet.  I did discover they beat the bound in the paper i gave you
> before.
> See
> http://dl.acm.org/citation.cfm?id=2756553&CFID=641345479&CFTOKEN=40648536
>
> The algorithm is also substantially simpler.
>
>
yes -- section 4.1 described what is needed. There is one problem though --
even though the algorithm handles arc deletion, the time bound mentioned
only applies to the arc addition.

>From the conclusion:

 "One may also ask whether arc deletions, instead of or in addition to
insertions,
can be handled. Our cycle detection and topological ordering algorithms
remain correct
if arcs can be deleted as well as inserted, but the time bounds are no
longer valid, and
we have no interesting bounds. Maintaining strong components as arcs are
deleted, or
as arcs are inserted and deleted, is an even more challenging problem."


david
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160708/2bd982ee/attachment.html>