[PATCH] D94982: [NPM][Inliner] Factor ImportedFunctionStats in the InlineAdvisor

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:15
 #include "llvm/IR/PassManager.h"
+#include "llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h"
 #include <memory>
----------------
davidxl wrote:
> Should the file be moved to the Analysis directory?
Maybe - I don't have a strong opinion. I'd do it in a separate patch, though - and see what folks think.


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:117
 
   bool Recorded = false;
 };
----------------
aeubanks wrote:
> drive by comment, this could be under `#ifdef NDEBUG`
ack, I'll patch separately, makes sense.


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:170
 
+  Module &M;
   FunctionAnalysisManager &FAM;
----------------
aeubanks wrote:
> is this used anywhere except the constructor?
MLInlineAdvisor, which had this member as a protected, and now is using this instead.


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:136
+void InlineAdvice::recordInlineStatsIfNeeded() {
+  if (InlinerFunctionImportStats != InlinerFunctionImportStatsOpts::No)
+    Advisor->ImportedFunctionsStats->recordInline(*Caller, *Callee);
----------------
aeubanks wrote:
> `if (Advisor->ImportedFunctionsStats)`?
Ya, that makes more sense (this was copy and paste old code)


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:447
+    : M(M), FAM(FAM) {
+  if (!ImportedFunctionsStats &&
+      InlinerFunctionImportStats != InlinerFunctionImportStatsOpts::No) {
----------------
davidxl wrote:
> Why is the first check needed?
ha! true. Copy'n'paste which now makes no sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94982