[PATCH] D24226: [PM] Provide an initial, minimal port of the inliner to the new pass manager.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm with minor nits



================
Comment at: lib/Transforms/IPO/Inliner.cpp:783
+  assert(InitialC.size() > 0 && "Cannot handle an empty SCC!");
+  Module &M = *InitialC.begin()->getFunction().getParent();
+  ProfileSummaryInfo *PSI = MAM.getCachedResult<ProfileSummaryAnalysis>(M);
----------------
Super minor, but it seems natural to have a `SCC::getModule()`.  Maybe in a later change?


================
Comment at: lib/Transforms/IPO/Inliner.cpp:808
+  // also need to use an updatable pointer for the SCC as a consequence.
+  SmallVector<LazyCallGraph::Node *, 16> Nodes;
+  for (auto &N : InitialC)
----------------
Why not `SmallVector<LazyCallGraph::Node *, 16> Nodes(InitialC.begin(), InitialC.end());`?


================
Comment at: lib/Transforms/IPO/Inliner.cpp:853
+    while (!Calls.empty()) {
+      CallSite CS = Calls.pop_back_val();
+      Function &Callee = *CS.getCalledFunction();
----------------
How about s/`Calls`/`CallsInCurrentNode`?  Given the nested loops, it is a little hard to keep track of whether `Calls` has all the call sites in the SCC or just the ones in the current node.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:868
+        if (Function *NewCallee = CS.getCalledFunction())
+          if (!NewCallee->isDeclaration())
+            Calls.push_back(CS);
----------------
(This is minor, please don't hold this patch over resolving this.  We can add this (or not) in a later change.)

I'd rather add to `Calls` if either `CS` is indirect OR is direct to a function with a body, since a later inline can make that call direct.

That way, we get this case:

```
fnptr_t f() { return printf; }
void g(fnptr_t val) {
  val("");
}
void main() {
  fnptr_t val = f();
  g(val);
}
```

If we're lucky enough to first inline `g` and then `f`, we want to consider inlining the call to `val` that came in from `g` (that is now directly, after inlining `f`).

Of course, we have to be somewhat smarter to always get this case, but the probability is higher if we reconsider indirect calls.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:915
+      if (InlinedCallee->hasLocalLinkage() && InlinedCallee->use_empty())
+        DeadFunctions.insert(InlinedCallee);
+    InlinedCallees.clear();
----------------
We'll only ever try to insert a function once into `DeadFunctions`, right?  Since if we're trying to insert `InlinedCallee` into `DeadFunctions` then it has no uses, and we could not possible try to inline through a call to it again later, so it should never appear again in `InlinedCallees`.

If that ^ is correct, perhaps we can add an assert here (and we could even use a `SmallVector` instead of `SmallPtrSet`, but I'd especially like to have the assert in that case).


https://reviews.llvm.org/D24226