[PATCH] D106448: [llvm][Inline] Add a module level inliner

[Kazu Hirata via Phabricator via llvm-commits]

kazu accepted this revision.
kazu added a comment.

Thank you for updating the patch.  The replay machinery changed since you uploaded your latest version.  Please see inline comments about that.

LGTM with the replay logic updated.  Thanks a lot for patiently updating multiple iterations!



================
Comment at: llvm/lib/Transforms/IPO/ModuleInliner.cpp:62-79
+static cl::opt<std::string> ModuleInlineReplayFile(
+    "module-inline-replay", cl::init(""), cl::value_desc("filename"),
+    cl::desc(
+        "Optimization remarks file containing inline remarks to be replayed "
+        "by inlining from module inline remarks."),
+    cl::Hidden);
+
----------------
I suggest you to remove the replay inliner logic, which is for debugging purposes.

The declaration of `ModuleInlineReplayScope` doesn't compile because of recent changes to the replay machinery.  You can forget about keeping up with the machinery simply by removing the replay logic here.


================
Comment at: llvm/lib/Transforms/IPO/ModuleInliner.cpp:118-122
+    if (!ModuleInlineReplayFile.empty())
+      OwnedAdvisor = getReplayInlineAdvisor(
+          M, FAM, M.getContext(), std::move(OwnedAdvisor),
+          ModuleInlineReplayFile, ModuleInlineReplayScope,
+          /*EmitRemarks=*/true);
----------------
I suggest you to drop this.


================
Comment at: llvm/lib/Transforms/IPO/ModuleInliner.cpp:147-148
+  auto &IAA = MAM.getResult<InlineAdvisorAnalysis>(M);
+  if (!IAA.tryCreate(Params, Mode, ModuleInlineReplayFile,
+                     ModuleInlineReplayScope)) {
+    M.getContext().emitError(
----------------
Replace the two replay parameters with `{}` like so:

```
  if (!IAA.tryCreate(Params, Mode, {})) {
```

The default-constructed instance has an empty string for its `ReplayFile` field and thus disables replay.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106448/new/

https://reviews.llvm.org/D106448