[PATCH] D79042: [llvm] Add interface to drive inlining decision using ML model

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

(just some minor stylistic/formatting things from me)



================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:103-112
+  std::function<AssumptionCache &(Function &)> GetAssumptionCache =
+      [&](Function &F) -> AssumptionCache & {
+    return FAM.getResult<AssumptionAnalysis>(Callee);
+  };
+  auto GetBFI = [&](Function &F) -> BlockFrequencyInfo & {
+    return FAM.getResult<BlockFrequencyAnalysis>(F);
+  };
----------------
Any particular reason that GetAssumptionCache is a std::function, but GetBFI/TLI/InlineCost are all lambdas without the std::function wrapper? (probably good to make GetAssumptionCache match the others, I guess)


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:124-125
+  auto OIC = llvm::shouldInline(CB, GetInlineCost, ORE);
+  std::unique_ptr<InlineAdvice> Ret(
+      new DefaultInlineAdvice(this, CB, OIC, ORE));
+  return Ret;
----------------
Prefer `std::make_unique` if/where possible. (probably written as `auto Ret = std::make_unique<DefaultInlineAdvice>(this, CB, OIC, ORE)` or just `return std::make_unique...;`)


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:142
+  for (auto *F : DeletedFunctions)
+    delete (F);
+  DeletedFunctions.clear();
----------------
Drop the `()` around `F` here - they're not needed. (also - elsewhere in this patch, there's a bunch of unnecessary `;` on function definitions (eg: `virtual void OnPassExit(){};`) - be good to remove those too)


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:678
+    OwnedDefaultAdvisor.emplace(getInlineParams());
+    return OwnedDefaultAdvisor.getValue();
+  }
----------------
Usually I'd prefer/suggest using `*` rather than `getValue()` on an `Optional`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79042