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

[Chandler Carruth via llvm-commits]

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 replied to that email, see my responses there.


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

I have broken this up to the greatest extent I was able to. I'm happy to
have suggestions for further splits (will reply below).


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

So, there were several discussions back when the LazyCallGraph was written.
That was some time ago, and so I understand folks don't recall them (I
don't recall all of them!). I'm working to capture them and put them into a
document. =] No need to +1 that a million times.


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

You might take a look at the unit tests for the LazyCallGraph. That code
went in incrementally, and with unit tests exactly as you describe to
exercise the abstract graph changes.

This patch is different. It is handling exactly *one* update scenario of
running function passes.

I understand that it is more code than people would naively expect. I share
that feeling. Even the old call graph's analog to this logic is 200 lines
of code and it handles fewer cases. =/


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

I think this is actually trying to follow the exact approach you've
outlined. I understand that the result may be somewhat ... frustrating. It
was for me. But I'm not sure how to effectively split up this logic.

For example, separating the devirt changes from the DCE changes don't make
a lot of sense -- they're both intrinsically just "function local changes
occurred" and are likely to be handled by the same piece of code. I've
actually separated out the inliner changes from this one. I'm hoping to do
that in a follow-up patch.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160707/bf278883/attachment.html>