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

[Daniel Berlin via llvm-commits]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160708/9900f08f/attachment.html>