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

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.
This revision is now accepted and ready to land.

lgtm, thanks.



================
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();
----------------
modimo wrote:
> 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.
Yes, it was functionally correct, but logically wrong.. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112040