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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Overall this looks great!  Minor comments inline.



================
Comment at: include/llvm/Transforms/IPO/Inliner.h:96
+public:
+  InlinerPass(InlineParams Params = llvm::getInlineParams())
+      : Params(std::move(Params)) {}
----------------
The explicit `llvm::` qualification is not needed.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:667
 /// Remove dead functions that are not included in DNR (Do Not Remove) list.
-bool Inliner::removeDeadFunctions(CallGraph &CG, bool AlwaysInlineOnly) {
+bool LegacyInlinerBase::removeDeadFunctions(CallGraph &CG, bool AlwaysInlineOnly) {
   SmallVector<CallGraphNode *, 16> FunctionsToRemove;
----------------
Line looks too long, clang-format?


================
Comment at: lib/Transforms/IPO/Inliner.cpp:781
+  Module &M = *InitialC.begin()->getFunction().getParent();
+  auto *PSI = MAM.getCachedResult<ProfileSummaryAnalysis>(M);
+
----------------
I'd mildly prefer spelling out `ProfileSummaryInfo` here, since the type is not obvious from context.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:799
+    auto &CalleeTTI = FAM.getResult<TargetIRAnalysis>(Callee);
+    return llvm::getInlineCost(CS, Params, CalleeTTI, GetAssumptionCache, PSI);
+  };
----------------
s/`llvm::getInlineCost`/`getInlineCost`/


================
Comment at: lib/Transforms/IPO/Inliner.cpp:857
+      // Add any new callsites to the worklist.
+      Calls.append(IFI.InlinedCallSites.begin(), IFI.InlinedCallSites.end());
+
----------------
I'm not confident that the calls sites you've put in `Calls` here and above will stay valid across the future iterations of this loop.  That is, say we're looking at the `main` SCC in:

```
void f() { return false; }
void g() { }
void main() {
  if (f() /* CS0 */)
    g() /* CS1 */;
}
```

Before entering the loop, we'll put `CS0` and `CS1` in `Calls`.  Once we inline through `CS0`, it is reasonable (though I don't know if it does this today) for `InlineFunction` to want to simplify away and delete `CS1`, leaving a dangling pointer in `Calls`.

Is there a reason why that ^ won't happen?


================
Comment at: lib/Transforms/IPO/Inliner.cpp:865
+        // To check this we also need to nuke any dead constant uses (perhaps
+        // made dead by this operation on other functions).
+        Callee.removeDeadConstantUsers();
----------------
This bit needs a unit test -- nothing in `make check` failed when I commented this out.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:895
+    // re-use the exact same logic for updating the call graph to reflect the
+    // change..
+    C = &updateCGAndAnalysisManagerForFunctionPass(CG, *C, N, AM, UR,
----------------
Two periods intentional?


================
Comment at: lib/Transforms/IPO/Inliner.cpp:897
+    C = &updateCGAndAnalysisManagerForFunctionPass(CG, *C, N, AM, UR,
+                                                   /*DebugLogging*/ true);
+    RC = &C->getOuterRefSCC();
----------------
Why is `DebugLogging` true by default?


https://reviews.llvm.org/D24226