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

Thu Jul 7 14:08:37 PDT 2016

On Thu, Jul 7, 2016 at 1:54 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:

>
> On Jul 7, 2016, at 1:48 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
>
> On Thu, Jul 7, 2016 at 11:18 AM, Mehdi Amini via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>>
>> On Jun 30, 2016, at 4:26 PM, Xinliang David Li via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>
>>> I would still really love to talk about any concerns or problems you see
>>> with the design. Currently the one I'm thinking about the most is quadratic
>>> behaviors, but if there are other concerns I'd like to discuss them.
>>>
>>
>>
>> I may be repeating myself, but I would like to summarize my concerns and
>> position here:
>>
>> 1. We have a goal to make the pass manager to use the new PM. There are
>> many known/concrete performance improvements depending on that (there is
>> possible short term solution that does not depend on new PM, but we've
>> decided to make them depend on new PM switch anyway).  Note the PM switch
>> is already a very large task with potential risks, coupling that task with
>> another huge change is not the right plan going forward.  The right way is
>> to make *minimal* changes enable new PM as soon as possible and make
>> incremental improvement of other parts later.  The longer we delay, the
>> longer the community suffers.
>>
>>
>> I agree (and I suspect everyone does), but this ship sailed more than 2
>> years ago :)
>> This should have been an initial process goal, but the current situation
>> is that most of it is implemented as far as I can tell, and I wouldn’t vote
>> for Chandler to stop his work while being close to land it.
>>
>
> This patch may be "close to land" (although that is arguable), but we
> haven't seen what kinds of changes this implies for e.g. the inliner. From
> my experience preparing D21921, I'm skeptical about the ability to
> reasonably maintain the inliner with Chandler's new stuff coexisting with
> the old PM. I would need to see a proof of concept that is sane before
> LGTM'ing Chandler's changes here (ignoring all the other points that
> David/Pete have made about doing this incrementally, testing all the
> various cases, getting a design document, etc.).
>
> On the other hand, the new PM's design makes it relatively easy for
> Chandler's stuff (once we have proper design docs, testing, PoC for the
> inliner, etc.) to live alongside the approach in D21921 if some people feel
> strongly that work for that needs to be done immediately (something that I
> disagree with, but for the sake of reaching consensus would not oppose).
>
>
>> (At the same time, I like Sean plan to get in production the new PM with
>> the current visitation order)
>>
>>
>> 2. Changes with this scale needs to be justified with concrete benefit.
>> If there are anything real missing, we should have known them already.
>>
>> 3. With changes in core infrastructure like this, subtle bugs are likely
>> to be very hard to debug. When a bug is introduced, there will no way for
>> user to workaround it (such as by disabling a pass). This is reason why
>> core piece like this needs to kept simple
>>
>>
>> 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.
>>
>> Having a document like this also helps reviewers of the code (for
>> reasoning of the correctness).  Without this, I suspect very few people can
>> help with maintaining the code in the future.
>>
>>
>> I Agree.
>>
>>
>>
>> 5.  Another added complexity is that the SCC passes are now required to
>> do the updates -- it seems pretty obscure to the pass writers.
>>
>>
>> We don’t have much SCC passes though, and I guess we can come up with
>> some nice way to express it.
>>
>
> Existing CGSCC passes already do updates. The hard part here is sharing
> code that does updates when the semantics of the updates are different in
> the old PM and the new PM. Even just look at PostOrderFunctionAttrs and how
> much code had to be duplicated, and that pass doesn't even mutate the call
> graph. It is going to be hard to paper over this and I would have to see a
> proof of concept to convince me otherwise.
>
> Or to summarize: I think the obvious incremental approach here is to start
> by using the old PM visitation logic, delete the old PM from the
> middle-end, then make changes to the visitation semantics without being
> constrained by coexisting with the old PM.
> Does anyone disagree with that?
>
>
> I don’t.
> Can you clarify if it prevents in any way Chandler from moving on with his
> work in any way or the two will be able to live together with an option to
> switch between the two? (It seems easily possible from your comments in
> this thread and the patch you submitted).
>

It does not significantly prevent Chandler from moving forward with this
work. The new PM's design specifically allows coexistence of various
different pass managers.

-- Sean Silva

>
> Thanks,
>
> —
> Mehdi
>
>
>
> -- Sean Silva
>
>
>>
>>
>> 6. What is the plan to address the quadratic behavior?
>>
>> 7. Is this tested on large programs (e.g, in LTO mode)?  How large is the
>> RefSCC graph? How much memory overhead is expected? What is the compile
>> time impact?
>>
>> 8. This change makes new PM transition not NFC.  It can potentially
>> affect a lot of other people, e.g., introduced performance regression,
>> correctness issue etc.
>>
>> So in short, my proposal is that we do minimal work in CGSCC pass manager
>> to enable/unblock porting of inliner to the the new PM, and we can
>> revisit/re-examine this patch in depth later when new PM is in place.  Does
>> it make sense?
>>
>>
>> Does it line up with Sean’s work?
>>
>> —
>> Mehdi
>>
>>
>>
>> thanks,
>>
>> David
>>
>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>> Regardless, if/when the evidence appears for those cases being
>>>>>> important to optimize, we can add them; porting to the new PM is orthogonal.
>>>>>>
>>>>>> [*] Hopefully I will start an RFC and send out a proof of concept
>>>>>> patch tomorrow; I just need to test out a proof of concept port of the
>>>>>> inliner on something bigger to make sure everything works.
>>>>>>
>>>>>
>>>>> As I have said in this thread, several other threads when
>>>>> LazyCallGraph was first designed, and in several other discussions, I
>>>>> continue to think this is the wrong approach.
>>>>>
>>>>
>>>> Can you provide links to those discussions? I can't find anything in my
>>>> email.
>>>>
>>>
>>> I have tried very hard to find them, and haven't succeeded. Really sorry
>>> about this. I know some of them took place outside of email sadly, but my
>>> memory is that some did take place in the discussion threads around the
>>> pass manager and the need for having a cache key for SCCs.
>>>
>>> That said, my only real goal in mentioning this is that I don't want you
>>> or others to think that this is something that was never discussed. I even
>>> clearly remember Hal having reservations about this. ;] I know I myself
>>> would have preferred to maintain precise similarity to the current pass
>>> manager, but I don't see a way that results in a robust interaction with
>>> cache based management of analyses.
>>>
>>> Anyways, I'm happy to try to explain this freshly if useful, but I'm not
>>> sure what the remaining points of contention or confusion are.
>>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160707/270fc67d/attachment.html>