[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/ReplayInlineAdvisor.h:30
+  Scope ReplayScope;
+  Fallback ReplayMode;
+  CallSiteFormat ReplayFormat;
----------------
> We can rename Mode -> Fallback, and xxx-inline-replay-mode -> xxx-inline-replay-fallback.

I meant to say we should rename all "Mode" to be Fallback, beyond the switch. 


================
Comment at: llvm/include/llvm/Analysis/ReplayInlineAdvisor.h:47
 private:
+  bool hasRemarksForFunction(Function &F) const {
+    return (ReplaySettings.ReplayScope ==
----------------
I thought I commented somewhere - hasInlineAdvice is a better name. When scope is module, we may not have remark for this function, and yet it can return true.


================
Comment at: llvm/lib/Analysis/ReplayInlineAdvisor.cpp:126-129
+    else if (ReplaySettings.ReplayMode ==
+             ReplayInlinerSettings::Fallback::NeverInline)
+      return std::make_unique<DefaultInlineAdvice>(this, CB, None, ORE,
+                                                   EmitRemarks);
----------------
Why is this a `Nona` instead of `llvm::InlineCost::getNever(..)`? Same for the other instance above.


================
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();
----------------
wenlei wrote:
> 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 ..
> ```
Actually what I suggested earlier was wrong based on current implementation of hasRemarksForFunction/hasInlineAdvice. That was assuming hasRemarksForFunction only returns true for explicit replay decision, and false for fall back cases. 

Perhaps something like this would be a clean structure:

```
// This inline decision is out of scope for replay advisor
if (!hasInlineAdvice(..)) {
  return {}
}

// Use explicit replay decision if available
Optional<bool> ShouldInline = getAdviceFromRemarks();
if (ShouldInline.hasValue()) {
  if (ShouldInline.getValue()) {
    ..
  }
  else {
   ..
  }
}

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

else if (fall back is never inline)

else 
   assert(fall back is original)

return ..
```

getAdviceFromRemarks looks up InlineSitesFromRemarks and returns three-way value.




================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:880-883
+      if (!Advice)
+        continue;
+
+      if (!Advice->isInliningRecommended()) {
----------------
Why we do we skip recordUnattemptedInlining when not not inlining due to not having an advice?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112040