[PATCH] D16802: [LCG] Construct an actual call graph with call-edge SCCs nested inside reference-edge SCCs.
Sanjoy Das via llvm-commits
llvm-commits at lists.llvm.org
Sun Feb 7 10:47:48 PST 2016
sanjoy added a comment.
First round of comments and questions (I'll do a second pass soon):
(PS: I haven't reviewed the tests yet)
================
Comment at: include/llvm/Analysis/LazyCallGraph.h:451
@@ +450,3 @@
+
+ range slice(SCC &BeginC, SCC &EndC) const {
+ assert(find(BeginC) <= find(EndC) &&
----------------
I don't see where `slice`, `Begin` and `End` above are used.
================
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));
}
----------------
Why can't `Parents` be a container of `const RefSCC *`?
================
Comment at: include/llvm/Analysis/LazyCallGraph.h:555
@@ +554,3 @@
+
+ /// Insert an edge whose parent is in this SCC and child is in some child
+ /// SCC.
----------------
s/SCC/RefSCC
================
Comment at: include/llvm/Analysis/LazyCallGraph.h:558
@@ -375,3 +557,3 @@
///
/// There must be an existing path from the caller to the callee. This
/// operation is inexpensive and does not change the set of SCCs in the
----------------
Minor: Might want to change this to "existing path from \p SourceN to \p TargetN" since the edge being inserted is not necessarily a call edge.
================
Comment at: include/llvm/Analysis/LazyCallGraph.h:559
@@ -376,3 +558,3 @@
/// There must be an existing path from the caller to the callee. This
/// operation is inexpensive and does not change the set of SCCs in the
/// graph.
----------------
Minor: "does not change the set of SCCs and RefSCCs" may be clearer (esp. since you use that language elsewhere).
================
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
----------------
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?
================
Comment at: lib/Analysis/LazyCallGraph.cpp:224
@@ +223,3 @@
+ if (&TargetSCC.getOuterRefSCC() == this) {
+ assert(SCCIndices.find(&TargetSCC)->second < i &&
+ "Edge between SCCs violates post-order relationship.");
----------------
I'd remove the `&TargetSCC == &SourceSCC` clause, and instead just have this assert be `<= i`.
================
Comment at: lib/Analysis/LazyCallGraph.cpp:300
@@ +299,3 @@
+ // target SCC.
+ // 2) If the source SCC is now past the target SCC in the postorder sequenc,
+ // and thus the new edge will flow toward the start, we are done.
----------------
Nit: sequence
================
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())
----------------
Minor: I'd use `llvm::any_of` for the inner loop over the outgoing edges.
================
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.
----------------
Nit: "the correct post-order"
================
Comment at: lib/Analysis/LazyCallGraph.cpp:404
@@ +403,3 @@
+ continue;
+ if (SCCIndices[&EdgeC] <= SourceIdx)
+ // Not in the postorder sequence between source and target.
----------------
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.
================
Comment at: lib/Analysis/LazyCallGraph.cpp:462
@@ +461,3 @@
+
+// And we're done! Verify in debug builds that the RefSCC is coherent.
+#ifndef NDEBUG
----------------
Nit: indentation
================
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();
----------------
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`?
================
Comment at: lib/Analysis/LazyCallGraph.cpp:733
@@ +732,3 @@
+ // that when merging. We also return this to the caller to allow them to
+ // invalidate tinformation pertaining to these RefSCCs.
+ SmallVector<RefSCC *, 1> Connected;
----------------
Nit: spelling
================
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))
----------------
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?
================
Comment at: lib/Analysis/LazyCallGraph.cpp:1010
@@ +1009,3 @@
+ do {
+ Node *N;
+ edge_iterator I;
----------------
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.
================
Comment at: lib/Analysis/LazyCallGraph.cpp:1033
@@ +1032,3 @@
+ }
+ if (ChildN.DFSNumber == -1) {
+ // Check if this edge's child node connects to the deleted edge's
----------------
Might be useful to explicitly document (on the field) that `DFSNumber` for `RefSCC` (and `SCC`?) instances is `-1` for all nodes unless we're mid-DFS. Perhaps we can even stick this invariant in `verify()`?
================
Comment at: lib/Analysis/LazyCallGraph.cpp:1042
@@ +1041,3 @@
+ MarkNodeForSCCNumber(*N, RootPostOrderNumber);
+ while (!PendingRefSCCStack.empty())
+ MarkNodeForSCCNumber(*PendingRefSCCStack.pop_back_val(),
----------------
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`.
================
Comment at: lib/Analysis/LazyCallGraph.cpp:1055
@@ +1054,3 @@
+ // it.
+ // However, we do need to remove this RufSCC from its RefSCC's parent
+ // set.
----------------
Nit: "RefSCC"
================
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.");
----------------
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.
================
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(
----------------
[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`.
http://reviews.llvm.org/D16802
More information about the llvm-commits
mailing list