[PATCH] D112040: [InlineAdvisor] Add mode/format switches and negative remark processing to Replay Inliner

[Di Mo via Phabricator via llvm-commits]

modimo added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:41
 
-/// For Replay Inliner initialization
-enum class ReplayInlineScope : int { Function, Module };
+struct CallSiteFormat {
+  enum class Format : int {
----------------
mtrofin wrote:
> I think we shold start moving the replay stuff in its header (with possibly some refactoring of how we get to obtain an advisor)
Moved ReplayInlinerSettings to ReplayInlineAdvisor.h and forward declared it like `OptimizationRemarkEmitter` in InlineAdvisor.h


================
Comment at: llvm/lib/Analysis/ReplayInlineAdvisor.cpp:109-110
     if (Iter != InlineSitesFromRemarks.end()) {
-      InlineSitesFromRemarks[Combined] = true;
-      InlineRecommended = llvm::InlineCost::getAlways("previously inlined");
-    }
-  } else if (Scope == ReplayInlineScope::Function) {
+      if (InlineSitesFromRemarks[Combined])
+        InlineRecommended = llvm::InlineCost::getAlways("previously inlined");
+      ReplayMadeDecision = true;
----------------
wenlei wrote:
> Should we also return a negative decision (never inline) if `InlineSitesFromRemarks[Combined]` is false?
Good catch, yes we should


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1896-1902
     ExternalInlineAdvisor = std::make_unique<ReplayInlineAdvisor>(
-        M, *FAM, Ctx, /*OriginalAdvisor=*/nullptr, ProfileInlineReplayFile,
-        ProfileInlineReplayScope, /*EmitRemarks=*/false);
+        M, *FAM, Ctx, /*OriginalAdvisor=*/nullptr,
+        ReplayInlinerSettings{ProfileInlineReplayFile,
+                              ProfileInlineReplayScope,
+                              ProfileInlineReplayMode,
+                              {ProfileInlineReplayFormat}},
+        /*EmitRemarks=*/false);
----------------
wenlei wrote:
> Hmm.. now I'm a bit confused why the call to getReplayInlineAdvisor is removed in the other diff.  
TL;DR getReplayInlineAdvisor is the correct way to do this.

That came about due to how we query if the replay inliner has remarks. Originally (including v1 here) I used `hasRemarksForFunction` which doesn't exist in the base `InlineAdvisor` class so the advisor needed to specifically be a `ReplayInlineAdvisor`. After I made the changes in D112033 to just rely on a decision being returned this was no longer necessary.


================
Comment at: llvm/test/Transforms/SampleProfile/inline-replay.ll:74-77
 ; REPLAY-ERROR: error: Could not open remarks file:
 ; REPLAY-ERROR-SCOPE: for the --sample-profile-inline-replay-scope option: Cannot find option named 'function'!
+; REPLAY-ERROR-MODE: for the --sample-profile-inline-replay-mode option: Cannot find option named 'original'!
+; REPLAY-ERROR-FORMAT: for the --sample-profile-inline-replay-format option: Cannot find option named 'line'!
----------------
wenlei wrote:
> These are probably not very necessary as they're testing cl::opt/cl::value/clEnumValN, though there's no harm in having them. On the other hand, it might be good have a case to cover LineColumn 
>  or LineDiscriminator.
Added them above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112040