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

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