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

Sat Jul 2 05:22:08 PDT 2016

On Fri, Jul 1, 2016 at 2:36 PM, Pete Cooper via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> FWIW, I agree with all of David's points here.
>
> I've said in the past that the new PM is going to introduce behaviour
> differences, some of which will uncover bugs in later passes. The more we
> diverge behaviourally, the more of those we'll need to fix before we can
> switch to the new PM.
>
> Also, we don't tend to like 2600 line patches. Chandler you have often
> asked people to find a way to land things incrementally so I think that is
> appropriate here. You've also asked in the past for detailed design and
> analysis which I know you are providing more of as questions arise, but
> something of this magnitude needs plenty of discussion. If anything, you
> should continue the discussion on llvm-dev until everyone is happy with the
> design, *then* come back to this patch.
>
> If you do prefer to continue this route then fair enough. To make it an
> incremental series of patches, I propose the following:
> - implement the most basic Ref SCC generator
> - write unit tests to show it works
> - land that patch
> - Implement the refCC to SCC visitor, unit test and land that
> - add a unit test which fakes various kinds of inliner updates to the
> graph. Basically try to cover all of the updates you think will be required
> by the inliner. Each one can be reviewed to make sure it's a valid kind of
> update that folks think makes sense
> - same again with devirt code, updates, and tests
> - finally you should have enough in place and everyone should be happy
> that the unit tests cover everything that we can port the passes.
>
> I think something like the above gives us a much more incremental route to
> the goal. People can review each piece individually instead of one large
> patch.
>

Also, thankfully we only have 4 CGSCC passes so it is feasible to do
changes across all of them simultaneously in reasonably small patches.
(I say this based on the experience implementing the proof of concept in
http://reviews.llvm.org/D21921 which required touching all 4 of them)
Patches incrementally porting all of the CGSCC passes to a completely new
data structure / update logic *with the old PM gone* will be small enough
to be palatable.
However, patches that try to port these passes to a new call graph data
structure / update logic *while sharing code with the old pass manager* are
going to require contortions (I would need to see a complete end-to-end
proof of concept to convince me otherwise).

So there are basically two parallelizable tasks:
1. Push towards removing the old PM from the middle-end, following the
direction in http://reviews.llvm.org/D21921
2. A new design for the CGSCC visitation logic, opening the door to CGSCC
analysis caching etc.

Chandler can keep pushing forward on the design discussion, testing,
implementation, etc. for this new LazyCallGraph stuff (e.g. using an
approach like the bullet points you describe).

Porting the old PM visitation logic like http://reviews.llvm.org/D21921
allows us to make forward progress towards being able to eliminate the old
PM compatibility in the middle end and thus opening the path for moving to
the new stuff without contortions for sharing code. On the other hand,
Chandler stated in D16381 that porting the inliner to the new PM will
be "essentially
a re-write" and if so then again there is no problem proceeding with that
in parallel.

The old PM is the common enemy here. Moving forward with an approach
like D21921
imposes no more constraints on Chandler's work than coexisting with the old
PM.

Another possibility is for Chandler to temporarily put these CGSCC
improvements on hold and help push forward towards deleting the old PM from
the middle-end. There are still major work items that I think would benefit
from his design insight, like optnone/optbisect, bugpoint support,
debug-pass=Structure, and pass manager builder (the new PM doesn't know
about dependencies which has the potential to make this quite non-trivial).

-- Sean Silva

>
> Cheers
> Pete
>
> Sent from my iPhone
>
> On Jul 1, 2016, at 1:26 AM, 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.
>
> 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.
>
> 5.  Another added complexity is that the SCC passes are now required to do
> the updates -- it seems pretty obscure to the pass writers.
>
> 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?
>
> 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/20160702/2b498b25/attachment.html>