[PATCH] D16802: [LCG] Construct an actual call graph with call-edge SCCs nested inside reference-edge SCCs.
Chandler Carruth via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 9 01:45:41 PST 2016
chandlerc added a comment.
Updated resolving most of the code review comments. Some questions and responses below:
================
Comment at: include/llvm/Analysis/LazyCallGraph.h:451
@@ +450,3 @@
+
+ range slice(SCC &BeginC, SCC &EndC) const {
+ assert(find(BeginC) <= find(EndC) &&
----------------
sanjoy wrote:
> I don't see where `slice`, `Begin` and `End` above are used.
I'm expecting clients to want to do:
for (auto &C : make_range(RC.begin(), RC.find(SomeOldC)))
...
And thought it would be good to directly support this rather than forcing the use of make_range by providing:
for (auto &C : RC.slice(RC.Begin, SomeOldC))
...
It happened to end up cleaner to write the tests using direct iterators instead. I can add a unit test specifically for this API or I can wait to add the API until I have the first user?
================
Comment at: include/llvm/Analysis/LazyCallGraph.h:480
@@ -348,1 +479,3 @@
+ bool isChildOf(const RefSCC &C) const {
+ return Parents.count(const_cast<RefSCC *>(&C));
}
----------------
sanjoy wrote:
> Why can't `Parents` be a container of `const RefSCC *`?
I guess it can, but there are some places where we walk the parents container and we are really planning to mutate stuff... I was mostly trying to reduce the number of const_casts I have to write. I don't feel very strongly about any of this.
================
Comment at: include/llvm/Analysis/LazyCallGraph.h:585
@@ -391,3 +584,3 @@
///
/// FIXME: We could possibly optimize this quite a bit for cases where the
/// caller and callee are very nearby in the graph. See comments in the
----------------
sanjoy wrote:
> Was this FIXME for the postorder optimization we do in this change, or something else? If the former, then perhaps this FIXME should be removed now?
In this case, the postorder optimization doesn't apply.
This comment is about the fact that the DFS over the inverse DAG formed with the 'parents' sets is potentially quite far reaching. We could do some things to try to prune this space in common cases. That's all.
================
Comment at: include/llvm/Analysis/LazyCallGraph.h:727
@@ -522,3 +726,3 @@
/// iterator walk.
SCC *lookupSCC(Node &N) const { return SCCMap.lookup(&N); }
----------------
sanjoy wrote:
> Why can't this (i.e. `ContainingSCC`) live as a field in `Node`?
I was originally trying to avoid digging into the Node object. Essentially, to allow the DFS to just find the SCC from the address of the node.
But I'm not sure any more that this is the right tradeoff.
Either way, moving completely away from the map seems like it could be usefully separated into a follow-up change.
================
Comment at: lib/Analysis/LazyCallGraph.cpp:334
@@ +333,3 @@
+ for (Edge &E : N) {
+ assert(E.getNode() && "Must have formed a node within an SCC!");
+ if (!E.isCall())
----------------
sanjoy wrote:
> Minor: I'd use `llvm::any_of` for the inner loop over the outgoing edges.
For all_of, I tend to agree. But I'm not sure that this:
return std::any_of(C.begin(), C.end(), [&](Node &N) {
return std::any_of(N.call_begin(), N.call_end(), [&](Edge &E) {
assert(E.getNode() && "Must have formed a node within an SCC!");
return ConnectedSet.count(G->lookupSCC(*E.getNode()));
});
});
Is more readable than:
for (Node &N : C)
for (Edge &E : N.calls()) {
assert(E.getNode() && "Must have formed a node within an SCC!");
if (ConnectedSet.count(G->lookupSCC(*E.getNode()))
return true;
}
return false;
================
Comment at: lib/Analysis/LazyCallGraph.cpp:358
@@ +357,3 @@
+
+ // If the target doesn't connect to the source, then we've correct the
+ // post-order and there are no cycles formed.
----------------
sanjoy wrote:
> Nit: "the correct post-order"
I think this should be "corrected the post-order" (which I've made it) but check me.
================
Comment at: lib/Analysis/LazyCallGraph.cpp:404
@@ +403,3 @@
+ continue;
+ if (SCCIndices[&EdgeC] <= SourceIdx)
+ // Not in the postorder sequence between source and target.
----------------
sanjoy wrote:
> Minor: I'd use `SCCIndices.find(&EdgeC)->second` here, just so that we crash if `&EdgeC` didn't end up in `SCCIndices` due to a bug earlier.
Yea, much better.
================
Comment at: lib/Analysis/LazyCallGraph.cpp:568
@@ +567,3 @@
+ // every other node, so we have formed a cycle. Pull the entire DFS
+ // and pending stacks into it.
+ int OldSize = OldSCC.size();
----------------
sanjoy wrote:
> This special case here makes me slightly uncomfortable. Unless you think it is important for performance (or other reasons I don't see yet), perhaps we can get rid of the `// Force the target node to be in the old SCC.` bit above (so that the `ChildN.DFSNumber == -1` case is never "spuriously" taken), and instead down below add `SCCNodes` to `OldSCC` if it contains `TargetN`?
Well, this clearly doesn't change the big-O, but I think it pretty dramatically shifts the average case. Whenever we hit this, we skip visiting all other edges on the "pop" half of the DFS which should be a really significant savings.
Is there anything that would make you more comfortable with it? We do this optimization in two places so I'd like to get it right.
================
Comment at: lib/Analysis/LazyCallGraph.cpp:840
@@ +839,3 @@
+ "Cannot have a null node within a visited SCC!");
+ RefSCC &ChildRC = *G->lookupRefSCC(*E.getNode());
+ if (ConnectedSet.count(&ChildRC))
----------------
sanjoy wrote:
> Given that you've used this pattern a lot, perhaps the interface should be `Node &getNode()` (which asserts that node exists) and perhaps an `Node *getNodePtr()` or `bool hasNode()` interface for clients that want to handle edges that don't yet have a node?
I think you're totally right. Should that go here or in a follow-up patch?
================
Comment at: lib/Analysis/LazyCallGraph.cpp:1010
@@ +1009,3 @@
+ do {
+ Node *N;
+ edge_iterator I;
----------------
sanjoy wrote:
> Why not have this DFS be over `SCC` s as nodes? That way we won't waste cycles DFS'ing //inside// an SCC; and it fits in better with the "`SCC` 's nested within `RefSCC`" design.
I thought a lot about this, but I don't think it helps much. Let me see if I can explain why.
Ultimately, the DFS is actually over *edges*, and the edges are fundamentally attached to nodes. We could use the SCC as the "node" in the DFS, but we'd have to include both an edge_iterator and a node_iterator to mark the position in the DFS stack, and we'd still visit exactly the same number of edges.
So while it makes the code a bit awkward, I don't think we really lose anything by directly DFS-ing the nodes, and we get a significantly simpler edge iterator model.
================
Comment at: lib/Analysis/LazyCallGraph.cpp:1042
@@ +1041,3 @@
+ MarkNodeForSCCNumber(*N, RootPostOrderNumber);
+ while (!PendingRefSCCStack.empty())
+ MarkNodeForSCCNumber(*PendingRefSCCStack.pop_back_val(),
----------------
sanjoy wrote:
> As I said earlier, unless there are cases where this really matters, I'd rather not have this special case here; but instead have a check on `RefSCCNodes` to see if it should be put in a new SCC or into `TargetC`.
See above.
================
Comment at: lib/Analysis/LazyCallGraph.cpp:1173
@@ +1172,3 @@
+ if (!Result.empty())
+ assert(!IsLeaf && "This SCC cannot be a leaf as we have split out new "
+ "SCCs by removing this edge.");
----------------
sanjoy wrote:
> Can there be cases where `Result` is empty, `IsLeaf` is false, and `this` was a leaf `RefSCC` before `removeInternalRefEdge` was called? If not, we can get rid of `IsLeaf` and update `G->LeafRefSCCs` only if `Result` is non-empty.
No, there can't (as you indicate below).
================
Comment at: lib/Analysis/LazyCallGraph.cpp:1182
@@ -542,4 +1181,3 @@
// the leaf SCC list.
- if (!IsLeafSCC && !ResultSCCs.empty())
- G->LeafSCCs.erase(std::remove(G->LeafSCCs.begin(), G->LeafSCCs.end(), this),
- G->LeafSCCs.end());
+ if (!IsLeaf && !Result.empty())
+ G->LeafRefSCCs.erase(
----------------
sanjoy wrote:
> [Edit: also see above]
>
> Doesn't `!Result.empty()` imply `!IsLeaf` (from the assert above)?
>
> I think you need `if (!WasLeafBeforeEdgeRemoval && !Result.empty())`, but I think just checking for `!IsLeaf` will Do The Right Thing, since `std::remove` doesn't break if `this` isn't present in `G->LeafRefSCCs`.
Yea, the IsLeaf is essentially just a debug check. I've made it now in fact just a debug check and left a FIXME about the cost of relying on std::remove rather than knowing if this RefSCC was already a leaf RefSCC.
http://reviews.llvm.org/D16802
More information about the llvm-commits
mailing list