[PATCH] D36352: [LCG] Switch one of the update methods for the LazyCallGraph to support limited batch updates.
Chandler Carruth via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 8 04:41:38 PDT 2017
chandlerc marked 3 inline comments as done.
chandlerc added a comment.
Thanks for the review all! I've fixed the mentioned issues and am landing based on Sean's LGTM.
In https://reviews.llvm.org/D36352#834396, @silvas wrote:
> One way to break up this large RefSCC is to notice that the fanout from the vtable ref edges in the ctors/dtors is contributing a lot to it being so large. If those edges are made more precise, then it can reduce the size. For example, by breaking the vtable ref edges we prevent the ref edges to virtual GetMetadata methods and so the huge RefSCC breaks up as I mentioned earlier.
> This can basically be seen as increasing the precision of the reference visitation; right now, it just assumes that all transitively reachable function pointers are ref edges. This leads to a lot of false positives, as ref edges are just meant to be a conservative superset of all edges that we might eventually discover through static optimization as call edges. In this case, the C2 constructors for Foo, Baz, and Metadata3 can be seen to contain no indirect calls and all their callees are either external or contain no indirect calls. So we can avoid adding any ref edges at all. A simple context-insensitive bottom-up RPO tracking whether an indirect call instruction is present be enough for this case (it would be another cache-busting walk over all instructions in the module though, but that may be worth it to decrease the number of ref edges / ref edge operations).
> I'm sure the tradeoffs and stuff for doing this kind of "may call through this function pointer" analysis are well studied in the Java literature, but I'm not really very familiar with it. The main difference for our use case here is that external functions count as "contributes no ref edges" instead of "could dynamically call potentially everything" (since we aren't concerned with runtime calls, but rather a conservative superset of statically discoverable direct calls).
> But of course the point of this is to be fast and get rid of the most egregious false positive ref edges so something simple is probably enough.
Sadly, I think this is a bit harder than it seems... We rely on reference edges to track *propagation* as well. So you can imagine a function F that is a leaf function and returns a function pointer. All callers might also have no indirect calls, but if we can inline F into them and inline some *other* function into them (or into their callers) we can still end up collapsing to a call.
Essentially, I think we'd end up looking at the entire reachable partition of the graph for indirect calls, and would almost always find one. =/
But honestly, with the fixes coming, even this fairly extreme case becomes very fast to handle, so I'm not too worried about this.
Comment at: lib/Analysis/LazyCallGraph.cpp:1133-1142
+ // Collect the SCCs for the source and targets.
SCC &SourceC = *G->lookupSCC(SourceN);
- SCC &TargetC = *G->lookupSCC(TargetN);
- if (&SourceC == &TargetC)
+ SmallSetVector<SCC *, 4> TargetCs;
+ for (Node *TargetN : TargetNs)
+ // If all targets are in the same SCC as the source, because no call edges
> I found this a bit confusing. SourceC and TargetCs seem to be only used for this bailout here. So the section-delineating comment `// Collect the SCCs for the source and targets.` is a bit misleading (seems like it is collecting them for use later). Maybe just move the `If all targets are in the same SCC as the source ...` comment up to there and move the `if` a bit closer to make that clear.
> In another patch you may want to consider putting the tarjan walk below into a separate function which I think would make this easier to understand. The main reason I think it seems pretty important to do that is to make it clear to the reader that we aren't doing anything fancy using our knowledge of the SourceC and TargetCs and are just walking everything in the RefSCC again, which makes the complexity obvious. (maybe beefing up the comments would help too)
> (then again, I can see why you'd want to keep it all inline, because above you raw delete edges and so some of the invariants might be broken until we finish the update.)
Yeah, this ended up not being relevant and being confusing. I've switched it to a much simpler and much more direct test for the early exit.
Regarding the longer-term issue of separating the Tarjan walk out, subsequent patches are likely to make this slightly less appealing, but I'm happy to revisit this in a subsequent patch if it makes sense.
More information about the llvm-commits