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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:49
@@ +48,3 @@
+/// reflects the nature of the update. For example, if an update splits one SCC
+/// into three SCCs, we also need to know the post-order traversal of those new
+/// SCCs.
----------------
Perhaps describing how this is handled (with an example?)

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:60
@@ +59,3 @@
+/// in addition to the layer of explicit function call edges. Because
+/// function-local and pairwise interprocedural transformations primarily turn
+/// an existing (possibly indirect) reference to a function into a direct call
----------------
Perhaps add an example here to show 'this is accomplished by ..'

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:72
@@ +71,3 @@
+///
+/// The extra ordering constraint also serves a secondary but closely related
+/// purpose. It allows a pass manager to identify regions of the call graph
----------------
This depends how secondary order constraints are formed (i.e., reference edges to potential targets are originated from callers of indirect calls or from constructor methods that reference vtables etc). Perhaps an example to demonstrate this?

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:136
@@ -77,1 +135,3 @@
+/// call graph is a subgraph of the reference graph, and thus the DAG of SCCs
+/// in the call graph is a subgraph of the DAG of SCCs in the reference graph.
 ///
----------------
This does not seem precise. The SCC nodes in RefGraph does not necessarily form a DAG (only RefSCCs), there can be cycles.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:142
@@ +141,3 @@
+/// formation is done lazily and on-demand during the traversal, including any
+/// necessary inspection of the IR to build the graph itself, in order to
+/// maximize locality when walking a module of the IR. The formation uses both
----------------
Is Laziness essential to the algorithm? If not, we don't need to emphasize it here -- just a footnote at the end should be enough.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:150
@@ +149,3 @@
+/// escaped into external 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 form even a potential call edge from a function body which may
----------------
Perhaps just define 'roots' as those callgraph nodes that can potentially called by external functions without body of IR that are available to the compilation.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:167
@@ +166,3 @@
+/// with the update. While in the worst case this includes the entire traversed
+/// region of the graph, hitting that worst case requires conneting a leaf of
+/// the graph to the traversed root in order to form a large cycle. If doing
----------------
Probably needs more description of updates : various scenarios (small example) and description on how each scenario is handled.


https://reviews.llvm.org/D22298