This is an archive of the discontinued LLVM Phabricator instance.

[InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner
ClosedPublic

Authored by modimo on Oct 18 2021, 5:11 PM.

Details

Summary

Adds the following switches:

  1. --sample-profile-inline-replay-fallback/--cgscc-inline-replay-fallback: controls what the replay advisor does for inline sites that are not present in the replay. Options are:
    1. Original: defers to original advisor
    2. AlwaysInline: inline all sites not in replay
    3. NeverInline: inline no sites not in replay
  1. --sample-profile-inline-replay-format/--cgscc-inline-replay-format: controls what format should be generated to match against the replay remarks. Options are:
    1. Line
    2. LineColumn
    3. LineDiscriminator
    4. LineColumnDiscriminator

Adds support for negative inlining decisions. These are denoted by "will not be inlined into" as compared to the positive "inlined into" in the remarks.

All of these together with the previous --sample-profile-inline-replay-scope/--cgscc-inline-replay-scope allow tweaking in how to apply replay. In my testing, I'm using:

  1. --sample-profile-inline-replay-scope/--cgscc-inline-replay-scope = Function to only replay on a function
  2. --sample-profile-inline-replay-fallback/--cgscc-inline-replay-fallback = NeverInline since I'm feeding in only positive remarks to the replay system
  3. --sample-profile-inline-replay-format/--cgscc-inline-replay-format = Line since I'm generating the remarks from DWARF information from GCC which can conflict quite heavily in column number compared to Clang

An alternative configuration could be to do Function, AlwaysInline, Line fallback with negative remarks which closer matches the final call-sites. Note that this can lead to unbounded inlining if a negative remark doesn't match/exist for one reason or another.

Updated various tests to cover the new switches and negative remarks

Testing:
ninja check-all

Diff Detail

Event Timeline

modimo created this revision.Oct 18 2021, 5:11 PM
modimo requested review of this revision.Oct 18 2021, 5:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2021, 5:11 PM
modimo edited the summary of this revision. (Show Details)Oct 18 2021, 5:22 PM
modimo added reviewers: wenlei, mtrofin.
wenlei added inline comments.Oct 21 2021, 1:40 PM
llvm/include/llvm/Analysis/InlineAdvisor.h
49

nit: OutputColumn -> outputColumn, same for OutputDiscriminator

65

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.

llvm/lib/Analysis/ReplayInlineAdvisor.cpp
101–149

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 ..
102–103

Use hasInlineAdvice (hasRemarksForFunction)?

127–144

Should we also return a negative decision (never inline) if InlineSitesFromRemarks[Combined] is false?

llvm/lib/Transforms/IPO/SampleProfile.cpp
1921–1928

Hmm.. now I'm a bit confused why the call to getReplayInlineAdvisor is removed in the other diff.

llvm/test/Transforms/SampleProfile/inline-replay.ll
63

AlwayLineColumnsInline -> LineColumn

101–105

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.

mtrofin added inline comments.Oct 21 2021, 2:21 PM
llvm/include/llvm/Analysis/InlineAdvisor.h
41–42

I think we shold start moving the replay stuff in its header (with possibly some refactoring of how we get to obtain an advisor)

modimo updated this revision to Diff 382861.Oct 27 2021, 5:22 PM
modimo marked 6 inline comments as done.

Review feedback

llvm/include/llvm/Analysis/InlineAdvisor.h
41–42

Moved ReplayInlinerSettings to ReplayInlineAdvisor.h and forward declared it like OptimizationRemarkEmitter in InlineAdvisor.h

llvm/lib/Analysis/ReplayInlineAdvisor.cpp
127–144

Good catch, yes we should

llvm/lib/Transforms/IPO/SampleProfile.cpp
1921–1928

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.

llvm/test/Transforms/SampleProfile/inline-replay.ll
101–105

Added them above

wenlei added inline comments.Oct 27 2021, 8:03 PM
llvm/include/llvm/Analysis/ReplayInlineAdvisor.h
30

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.

76

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.

llvm/lib/Analysis/ReplayInlineAdvisor.cpp
101–149

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.

152–155

Why is this a Nona instead of llvm::InlineCost::getNever(..)? Same for the other instance above.

llvm/lib/Transforms/IPO/Inliner.cpp
881–885

Why we do we skip recordUnattemptedInlining when not not inlining due to not having an advice?

Change summary needs to be updated as well.

modimo retitled this revision from [InlineAdvisor] Add mode/format switches and negative remark processing to Replay Inliner to [InlineAdvisor] Add fallback/format switches and negative remark processing to Replay Inliner.Oct 27 2021, 9:18 PM
modimo edited the summary of this revision. (Show Details)
modimo updated this revision to Diff 382905.Oct 27 2021, 9:24 PM

ReplayMode->ReplayFallback, refactoring

llvm/include/llvm/Analysis/ReplayInlineAdvisor.h
30

I missed that one, thanks.

76

You commented that in the other change (D112033). Still makes sense, changed.

llvm/lib/Analysis/ReplayInlineAdvisor.cpp
101–149

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.

152–155
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. I'll add comments to that effect.

llvm/lib/Transforms/IPO/Inliner.cpp
881–885

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.

wenlei accepted this revision.Oct 27 2021, 10:07 PM

lgtm, thanks.

llvm/lib/Analysis/ReplayInlineAdvisor.cpp
101–149

Yes, it was functionally correct, but logically wrong..

This revision is now accepted and ready to land.Oct 27 2021, 10:07 PM
modimo updated this revision to Diff 382914.Oct 27 2021, 10:26 PM

Fix up comment

mtrofin added inline comments.Oct 28 2021, 7:52 AM
llvm/include/llvm/Analysis/InlineAdvisor.h
235

should ReplayInlinerSettings be passed as const &?

257–258

can this declaration also move to ReplayInlineAdvisor.h?

283

can formatting (so callsiteformat and getCallSiteLocation) be moved to ReplayInlineAdvisor.h?

also: s/getCallSiteLocation/formatCallSiteLocation.

llvm/lib/Transforms/IPO/Inliner.cpp
877–878

unrelated change?

modimo updated this revision to Diff 383199.Oct 28 2021, 4:31 PM
modimo marked 2 inline comments as done.

Move more things to ReplayInlineAdvisor.h and other feedback

llvm/include/llvm/Analysis/InlineAdvisor.h
283

Yes, this was originally added for replay and only that consumes this atm.

llvm/lib/Transforms/IPO/Inliner.cpp
877–878

The comment below is that !Advice isn't very clear with auto and also tripped me up: the previous change
if (!Advice || !Advice->isInliningRecommended()) {

treated !Advice as having "negative" advice rather than being empty. Clearing this up seems valuable.

mtrofin accepted this revision.Oct 29 2021, 8:15 AM

lgtm

llvm/lib/Transforms/IPO/Inliner.cpp
877–878

ah, good point, thanks!

modimo updated this revision to Diff 383464.Oct 29 2021, 12:30 PM

Rebase and add -disable-output consistently to inline-replay.ll

This revision was landed with ongoing or failed builds.Oct 29 2021, 12:32 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the reviews @mtrofin !