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

Wed Jun 29 00:03:20 PDT 2016

On Tue, Jun 28, 2016 at 5:41 PM, Chandler Carruth <chandlerc at gmail.com>
wrote:

> chandlerc added a comment.
>
> Digging into some of these comments, will get David's in the next email...
>
> In http://reviews.llvm.org/D21464#460784, @silvas wrote:
>
> > Some initial comments.
> >
> > You've described a lot of implementation details (and implementation),
> but haven't described the final intended visitation behavior you are trying
> to implement (or why). For starters, what's wrong with the old PM CGSCC
> visitation order that requires doing something fundamentally different in
> the new PM?
>
>
>
Thanks for the reply.

> I'll post an update to the patch that adds a much more detailed comment to
> the header to try and describe what this is trying to achieve at a high
> level, etc. It includes some of the why, but it doesn't going into very
> specific details. I'm happy to do so here and add the details you or others
> find sufficient relevant and not too brittle to go there.
>
> To summarize what is wrong with the old visit order: it misses specific
> optimization opportunities, and makes it harder to reason about the call
> graph updates in a principled way.

I think this is the key question to answer. What specific opportunities are
missed? Are there real  world examples other than test case programs?  Real
programs are usually complex enough that hypothetical benefits do not
materialize.

> The latter becomes more important when there might be cached analyses
> attached to the call graph nodes. Consider if we wanted to lift some of the
> things in FunctionAttrs into an *analysis* over an SCC rather than an
> attribute. Now it is essential that update to the graph structure
> invalidate the right set of analyses.
>

I am not sure what it means. Is it essential for correctness or for
precision?

>
> As an example of the kind of missed optimizations, I'm going to add a test
> case where we successfully deduce readnone with the new pass manager but
> can't (no matter how many iterations we make) deduce it with the old one.
> This is because we don't refine the SCC graph structure, and do principled
> visitation to that graph structure.

I saw the test case. The question is there anything preventing SCC graph be
refined for the second iteration in the old framework?

If we are really keen in solving this problem, another way is to do a
simple module pass to do the cleanup before the SCC is built. This will at
least work for the examples in the test.

> The examples in the test case are somewhat contrived in order to make good
> test cases, but they hopefully represent the general pattern that I'm
> worried about here: we are using the set of functions in the SCC to reason
> about it as an SCC but not actually updating that set of functions when the
> code changes.
>

I understand the motivation. What I want to know  is the complexity/benefit
trade-offs here. Using refined attributes due to de-virtualization as
example is not convincing IMO.

thanks,

David

>
> > The general idea is clearly to maintain a bottom-up visitation behavior,
> but since this patch doesn't implement edge addition (which represents
> devirtualization and so is pretty important) the intended behavior there is
> unclear.
>
>
> So, this *does* implement ref -> call promotion which is what I would
> expect devirtualization to look like in all cases. See below for details on
> your question.
>
> The other case is for "out-of-thin-air" call (or ref) insertion such as
> instrumentation passes might do. Currently this isn't supported but could
> be added in the future.
>
>
> ================
> Comment at: lib/Analysis/CGSCCPassManager.cpp:90
> @@ +89,3 @@
> +    for (Instruction &I : BB)
> +      if (auto CS = CallSite(&I))
> +        if (Function *Callee = CS.getCalledFunction())
> ----------------
> (in case reading this against the current patch, this originally was
> attached to the FIXME regarding handling adding new calls)
>
> As i somewhat alluded to above, what you describe should be handled by a
> ref edge turning into a call edge, and the update mechanism should be able
> to handle that well.
>
> I've added a test case that exercises this with GVN and function-attrs.
> There is currently a case missed in the new pass manager because we don't
> have the up-to-four iteration whenever an indirect call turns into a direct
> call heuristic that the old pass manager has. I'm happy to add that, but
> I'd like to add it in a follow-up patch. I've marked where in the test case
> this is missed, and I've demonstrated that in theory this update mechanism
> is sufficient to handle it by explicitly running function-attrs again and
> it correctly catches the refinement.
>
> The direct answer to your question is #2: it continues running on the
> now-larger SCC, detects that we switch from one SCC to another at some
> point, and re-runs on that SCC to make sure that the refined graph is
> observed.
>
> ================
> Comment at: lib/Analysis/CGSCCPassManager.cpp:170
> @@ +169,3 @@
> +
> +    // The easy case is when the target RefSCC is not this RefSCC. This is
> +    // only supported when the target RefSCC is a child of this RefSCC.
> ----------------
> sanjoy wrote:
> > Doesn't this also hold if the target SCC is not this SCC?
> Yes, and inside the implementation of switchInternalEdgeToRef, it early
> exits with an empty range when we hit that scenario.
>
> I can lift that distinction up into this code if you think that would be
> helpful, the somewhat arbitrary split was that these methods are on a
> RefSCC, and so it is the RefSCC-easy case that callers have to handle.
>
>
> http://reviews.llvm.org/D21464
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160629/59c14096/attachment.html>