[PATCH] D16038: [LCG] Build an edge abstraction for the LazyCallGraph and use it to differentiate between indirect references to functions an direct calls.

[Chandler Carruth via llvm-commits]

chandlerc marked 5 inline comments as done.
chandlerc added a comment.

Thanks both!

Planning to submit soon, but please keep discussing any of the open issues here and I'll make sure to follow up with patches to correct anything. Also lemme know if my resolution isn't really satisfactory.


================
Comment at: include/llvm/Analysis/LazyCallGraph.h:109
@@ +108,3 @@
+
+  /// A class used to represent edges in the call graph.
+  ///
----------------
reames wrote:
> Having a class called edge which doesn't encode the source seems mildly confusing.  Possibly OutboundEdge?  Decidedly minor and optional.
> 
> If you make the change suggested, it might be good to pull enum out as a class enum Edge to keep the use sites descriptive.  Edge::Ref is more meaningful that OutgoingEdge::Ref to me.  Either way, nit, I'd be fine with either.
> 
> A bit more detail about the Function/Node and lazy creation might be useful for context on the accessors below.
> 
> 
Yea, I hear what you're saying here, but I think I'm going to keep this the shorter "edge" for now.

One reason why is that the way you *get* edges is from the source, and I think that will help keep it from being too confusing.

But the comment about more context is very helpful, I'll beef up the comments, but let me know if I get it detailed enough to help.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:137
@@ +136,3 @@
+    /// This requires that the edge is not null, but will succeed whether we
+    /// have bulit a call graph node for the function yet or not.
+    Function &getFunction() const;
----------------
reames wrote:
> built
> 
> Also, the last half of the comment doesn't parse when reading from the top of the diff?  Is this something which needs more explanation or is it obvious from something else in the code?
Yea, I think your context comment above was spot on. Hopefully addressed.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:263
@@ +262,3 @@
+        : iterator_adaptor_base(BaseI), E(E) {
+      while (I != E && !*I && !I->isCall())
+        ++I;
----------------
reames wrote:
> Could this be separated into a helper "advance_to_next_call" or something?  Might make the code more obvious.
> 
> Also, this looks like this stops on a null edge?  Why?
Meh, sure on the refactoring.

It should *continue* on null edges and non-call edges to find the first valid edge? Added to comment.

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:608
@@ +607,3 @@
+inline Function &LazyCallGraph::Edge::getFunction() const {
+  assert(*this && "Queried a null edge!");
+  auto P = Value.getPointer();
----------------
reames wrote:
> Consistency: tested vs queried four lines apart.
Tested -> a bool result
Queried -> a non bool result

Anyways, I don't care deeply, happy to make them all say queried...

================
Comment at: include/llvm/Analysis/LazyCallGraph.h:626
@@ +625,3 @@
+inline LazyCallGraph::Node &LazyCallGraph::Edge::getNode(LazyCallGraph &G) {
+  assert(*this && "Queried a null edge!");
+  auto P = Value.getPointer();
----------------
reames wrote:
> It looks like this can be:
> if (auto *Existing = getNode())
>   return Existing;
> Node &M = ...
I mean, it could, it just didn't seem to simplify anything, especially as I'll still need to query P to get the function out.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:26
@@ +25,3 @@
+                    LazyCallGraph::Edge::Kind EK) {
+  // Note that we consider *any* function with a definition to be a viable
+  // edge. Even if the function's definition is subject to replacement by
----------------
reames wrote:
> Why are we excluding edges to declarations?  That seems odd to me.  (Not directly related to this review and should not block.)
It is odd, and something I plan to add after we *actually* start modeling call edges correctly. Currently, this patch has us track which edges are calls but do nothing useful with them. Next step is to build useful graph structures with them. Then I plan to add support for declarations and flags indicating when there is a call to a totally unknown function.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:74
@@ +73,3 @@
+  // are trivially added, but to accumulate the latter we walk the instructions
+  // and add every operand which is a constant to the worklist to process
+  // afterward.
----------------
echristo wrote:
> s/constant/"direct call" perhaps?
Hmm, no, I really do mean constant here? We *don't* add direct calls to the worklist.

================
Comment at: lib/Analysis/LazyCallGraph.cpp:318
@@ +317,3 @@
+      for (Edge &E : *N) {
+        assert(E.getNode() && "Cannot have a null node within a visited SCC!");
+        SCC &ChildC = *G->SCCMap.lookup(E.getNode());
----------------
reames wrote:
> Hm, time for a getNodeRequired()?
Meh, but then it couldn't include the helpful context?


http://reviews.llvm.org/D16038