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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:49
+
+  bool OutputColumn() const {
+    return OutputFormat == Format::LineColumn ||
----------------
nit: `OutputColumn` -> `outputColumn`, same for `OutputDiscriminator`


================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:65
+  enum class Scope : int { Function, Module };
+  enum class Mode : int { Original, AlwaysInline, NeverInline };
+
----------------
I think this now reflects a narrower aspect of the settings than mode. Specifically, this is some kind of fall back inline strategy. I suggest we rename this field as well as the flag names because "AlwaysInline" reply mode doesn't really convey the right message, and can be confusing. 

We can rename `Mode` -> `Fallback`, and `xxx-inline-replay-mode` -> `xxx-inline-replay-fallback`.





================
Comment at: llvm/lib/Analysis/ReplayInlineAdvisor.cpp:99-137
+  // Decision could be made by replay system
+  if (ReplaySettings.ReplayScope == ReplayInlinerSettings::Scope::Module ||
       CallersToReplay.count(CB.getFunction()->getName())) {
-    std::string CallSiteLoc = getCallSiteLocation(CB.getDebugLoc());
+    std::string CallSiteLoc =
+        getCallSiteLocation(CB.getDebugLoc(), ReplaySettings.ReplayFormat);
     StringRef Callee = CB.getCalledFunction()->getName();
     std::string Combined = (Callee + CallSiteLoc).str();
----------------
always inline, never inline and original advisor are parallel, I think we can restructure the logic and add some comments to help readability:

```
// We have a decision from replay
if (hasInlineAdvice(..)) {

  return ...
}

// We need to fall back ..
if (always inline)

else if (never inline)

else {
  assert(original);
  
}

return ..
```


================
Comment at: llvm/lib/Analysis/ReplayInlineAdvisor.cpp:100-101
+  // Decision could be made by replay system
+  if (ReplaySettings.ReplayScope == ReplayInlinerSettings::Scope::Module ||
       CallersToReplay.count(CB.getFunction()->getName())) {
+    std::string CallSiteLoc =
----------------
Use hasInlineAdvice (hasRemarksForFunction)?


================
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;
----------------
Should we also return a negative decision (never inline) if `InlineSitesFromRemarks[Combined]` is false?


================
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);
----------------
Hmm.. now I'm a bit confused why the call to getReplayInlineAdvisor is removed in the other diff.  


================
Comment at: llvm/test/Transforms/SampleProfile/inline-replay.ll:42
+
+;; Check format inlining errors out on non <Line|AlwayLineColumnsInline|LineDiscriminator|LineColumnDiscriminator> inputs
+; RUN: not opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/inline-topdown.prof -sample-profile-inline-replay=%S/Inputs/inline-replay.txt -sample-profile-inline-replay-format=line -sample-profile-merge-inlinee -sample-profile-top-down-load -pass-remarks=inline -S 2>&1 | FileCheck -check-prefix=REPLAY-ERROR-FORMAT %s
----------------
AlwayLineColumnsInline -> LineColumn


================
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'!
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112040