[PATCH] D24225: [LCG] Add the necessary functionality to the LazyCallGraph to support inlining.

[Chandler Carruth via llvm-commits]

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

Thanks again for the review! Committing shortly and will make two follow-ups to address the comments that seem separable (either needing more utility code unrelated to this patch, and/or needing more tests).



================
Comment at: lib/Analysis/LazyCallGraph.cpp:1340
+  RefSCC &TargetRC = *G->lookupRefSCC(TargetN);
+  if (&TargetRC == this)
+    return;
----------------
sanjoy wrote:
> If we're inserting a call edge and `&TargetRC == this` then I think we should assert that the target `SCC` is equal to or a descendent of the source `SCC`.
> 
> Actually, perhaps we should assert the target `SCC` is equal to or a descendent of the source `SCC` whenever we're inserting a call edge (i.e. without regards to whether `&TargetRC == this`).  That'll be partially redundant with `TargetRC.isDescendantOf(*this)` below, but I think that's fine.
We don't have any utility code for checking if an SCC is a descendent of another SCC in the call graph, so writing these assertions is very verbose right now. So I'll plan add it in a follow-up with some utility code.


================
Comment at: lib/Analysis/LazyCallGraph.cpp:1408
+void LazyCallGraph::removeDeadFunction(Function &F) {
+  assert(F.use_empty() &&
+         "This routine should only be called on trivially dead functions!");
----------------
sanjoy wrote:
> How about recursive functions?  It seems reasonable to want to all `removeDeadFunction` on them too.
Interesting. I mean, yes, it does. And yet the caller I wanted to call this has this exact check before calling it.

But I agree that it seems like a meaningless restriction at *this* layer of the API.

I've left a FIXME here for now as I'd like to add this in a follow-up commit with its own test case.


================
Comment at: lib/Analysis/LazyCallGraph.cpp:1413
+  if (EII != EntryIndexMap.end()) {
+    EntryEdges[EII->second] = Edge();
+    EntryIndexMap.erase(EII);
----------------
sanjoy wrote:
> Any reason why you can't compact the `EntryEdges` vector here (i.e. swap the last element with the `EII->second` th element, pop the last element and update `EntryIndexMap`)?
It would make the iteration order dependent on the particular mutation order which seems ... unfortunate.

If the sparseness ever becomes an issue here, I'd like to add an ADT that handles this generically when growing the vector.


https://reviews.llvm.org/D24225