[PATCH] D112040: [InlineAdvisor] Add fallback/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/ReplayInlineAdvisor.h:30
+  Scope ReplayScope;
+  Fallback ReplayMode;
+  CallSiteFormat ReplayFormat;
----------------
wenlei wrote:
> > 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. 
I missed that one, thanks.


================
Comment at: llvm/include/llvm/Analysis/ReplayInlineAdvisor.h:47
 private:
+  bool hasRemarksForFunction(Function &F) const {
+    return (ReplaySettings.ReplayScope ==
----------------
wenlei wrote:
> 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.
You commented that in the other change (D112033). Still makes sense, changed.


================
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);
----------------
wenlei wrote:
> Why is this a `Nona` instead of `llvm::InlineCost::getNever(..)`? Same for the other instance above.
```
  DefaultInlineAdvice(InlineAdvisor *Advisor, CallBase &CB,
                      Optional<InlineCost> OIC, OptimizationRemarkEmitter &ORE,
                      bool EmitRemarks = true)
      : InlineAdvice(Advisor, CB, ORE, /*IsInliningRecommended=*/OIC.hasValue())
```

If the Optional cost has a value, IsInliningRecommended gets set to true. This looks [intended](https://github.com/llvm/llvm-project/blob/release/13.x/llvm/lib/Analysis/InlineAdvisor.cpp#L359). I'll add comments to that effect.


================
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:
> 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.
> 
> 
With the nesting of the CGSCC inliner inside this, no advice still defers to the original advisor if it exists. That above is cleaner though, changed.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:880-883
+      if (!Advice)
+        continue;
+
+      if (!Advice->isInliningRecommended()) {
----------------
wenlei wrote:
> Why we do we skip recordUnattemptedInlining when not not inlining due to not having an advice?
Advice is a std::unique_ptr<InlineAdvice> so being negative means it doesn't contain a value. Calling `recordUnattemptedInlining` on that leads to an AV. This path is untested ATM because the cgscc advisor is always nested inside the replay one so there will always be an `InlineAdvice` returned. I came upon this when one of my intermediate edits didn't forward the decision to the original advisor.

I made the `auto` explicit to make this clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112040