[PATCH] D22298: [LCG] Update and expand comments to properly document the design motivation, tradeoffs, and constraints.

[Sean Silva via llvm-commits]

silvas added a comment.

Some initial comments.


================
Comment at: include/llvm/Analysis/LazyCallGraph.h:35
@@ +34,3 @@
+/// updates and refinements made to it. There are two primary forms of call
+/// graph refinement we care about:
+///
----------------
Something like outlining will also add new nodes. You may want to mention that somewhere.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:117
@@ +116,3 @@
+/// NB: This is *not* a traditional call graph! It is a graph which models both
+/// the current calls and potential calls. As a consequence there are many
+/// edges in this call graph that do not correspond to a 'call' or 'invoke'
----------------
You may want to clarify the exact sense in which "potential" is meant here. The types of edges in a traditional call graph could also be considered as "potential".

Maybe one wording could be "current direct calls" and "references to other functions that might be turned into 'current direct calls' during static optimization"

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:126
@@ -77,10 +125,3 @@
 ///
-/// To this end, graph represents both direct and any potential resolution to
-/// an indirect call edge. Another way to think about it is that it represents
-/// both the direct call edges and any direct call edges that might be formed
-/// through static optimizations. Specifically, it considers taking the address
-/// of a function to be an edge in the call graph because this might be
-/// forwarded to become a direct call by some subsequent function-local
-/// optimization. The result is that the graph closely follows the use-def
-/// edges for functions. Walking "up" the graph can be done by looking at all
-/// of the uses of a function.
+/// The analysis builds a pruned call graph with an edge between two function
+/// if there exists at least one direct call from one to the other. On top of
----------------
You may want to spell out what you mean by "pruned" here (or is this common terminology I'm unaware of?). Since you define the exact circumstances in which an edge exists, I'm not sure that there's much benefit to saying "pruned" here though.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:128
@@ +127,3 @@
+/// if there exists at least one direct call from one to the other. On top of
+/// that is layered a pruned reference graph with an edge between two functions
+/// if there exists at least one (potentially transitive, through multiple
----------------
Is the "call" graph not a subgraph of the "reference" graph? If it isn't, you probably want to show a concrete example of a situation where it isn't.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:141
@@ +140,3 @@
+/// locality when walking a module of the IR. The formation uses
+/// straight-forward and lazy implementations of Tarjan's SCC detection
+/// algorithm, and so is linear in the number of edges plus nodes.
----------------
Does this sentence mean it uses two implementations? That it uses one which is both straight-forward and lazy? I don't see what "straight-forward" adds to this sentence nor how an implementation of Tarjan's algorithm that is lazy can be considered "straight-forward". Is there a specific thing you're trying to communicate?

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:145
@@ -88,3 +144,3 @@
 /// The roots of the call graph are the external functions and functions
 /// escaped into global variables. Those functions can be called from outside
 /// of the module or via unknowable means in the IR -- we may not be able to
----------------
small nit: this should probably be "escaped into external global variables"

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:159
@@ +158,3 @@
+///
+/// Updates are also implemented using straightforward versions of Tarjan's SCC
+/// finding algorithm and are expected to be linear in the number of edges
----------------
Tarjan's algorithm per se doesn't deal with updates and so saying that the updates are handled with"using straightforward versions of Tarjan's SCC finding algorithm" doesn't provide much information. Could you maybe give a concrete explanation? E.g. that you perform a DFS from ... reusing the LowLink's that have already been computed in such and such a way. Then you can draw parallels to Tarjan's algorithm.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:161
@@ +160,3 @@
+/// finding algorithm and are expected to be linear in the number of edges
+/// leaving the set of potentially impacted nodes plus the number of impacted
+/// nodes. While in the worst case this is the entire traversed region of the
----------------
Could you elaborate here about what "impacted" / "potentially impacted" means for the various kinds of updates?


http://reviews.llvm.org/D22298