[PATCH] D36352: [LCG] Switch one of the update methods for the LazyCallGraph to support limited batch updates.

Sean Silva via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 8 09:36:35 PDT 2017


On Aug 8, 2017 4:41 AM, "Chandler Carruth via Phabricator" <
reviews at reviews.llvm.org> wrote:

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.


Ah, of course! I wasn't thinking about that case but you're totally right.
That makes it a fair bit harder.

-- Sean Silva


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)
+    TargetCs.insert(G->lookupSCC(*TargetN));
+
+  // If all targets are in the same SCC as the source, because no call
edges
----------------
silvas wrote:
> 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.


https://reviews.llvm.org/D36352
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170808/d7fdf9ef/attachment.html>


More information about the llvm-commits mailing list