[PATCH] D83743: [InlineAdvisor] New inliner advisor to replay inlining from optimization remarks

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:184
+/// previous build to guide current inlining. This is useful for inliner tuning.
+class ReplayInlineAdvisor : public InlineAdvisor {
+public:
----------------
Nit: could this be factored in its own .h file (and .cpp)?


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:189
+  std::unique_ptr<InlineAdvice> getAdvice(CallBase &CB) override;
+  bool ReplayRemarksLoaded() const { return HasReplayRemarks; }
+
----------------
nit: lower case first letter, also verb form (e.g. areReplayRemarksLoaded)


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:193
+  StringSet<> InlineSitesFromRemarks;
+  bool HasReplayRemarks;
+};
----------------
Nit: initialize HasReplayRemarks here, then the state of the object is deterministic.


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:162
+      CallSiteLoc << " @ ";
+    unsigned int Offset =
+        DIL->getLine() - DIL->getScope()->getSubprogram()->getLine();
----------------
this assumes DIL->getLine() >= DIL->getScope()->getSubprogram()->getLine(). Perhaps an assert before, or Offset could be int, and check it's non-negative?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:329
 
-  bool doInitialization(Module &M);
+  bool doInitialization(Module &M, FunctionAnalysisManager *FAM = nullptr);
   bool runOnModule(Module &M, ModuleAnalysisManager *AM,
----------------
I'm curious, is there a case when FAM would be not passed? (i.e. why not just require it?)


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:911
 bool SampleProfileLoader::inlineCallInstruction(CallBase &CB) {
+  std::unique_ptr<InlineAdvice> Advice = nullptr;
+  if (ExternalInlineAdvisor) {
----------------
why not define Advice within the if below?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83743