[PATCH] D94333: [Inliner] Change inline remark format and update ReplayInlineAdvisor to use it

[Di Mo via Phabricator via llvm-commits]

modimo added a comment.

The modifications to the DefaultInlineAdvice is an attempt to solve the following problem:

1. In the SampleProfile inliner it emits remarks through the "legacy" interface with `emitInlinedInto`. For replay if advice generates remarks you'll get duplicate remarks but if it doesn't you get a single set (which is what's happening today and correct).
2. In the CGSCC inliner in newPM it emits remarks through InlineAdvice via `recordInlining` and nowhere else. For replay if InlineAdvice generates remarks you're good but if it doesn't you get no remarks.

So to solve this issue I have the ReplayAdvisor use DefaultInlineAdvice with modifications to suppress remarks for the SampleProfile inliner.

In D94333#2488360 <https://reviews.llvm.org/D94333#2488360>, @mtrofin wrote:

> I can look into it, we need to look at its uses on the ML side. Perhaps the logging part can be moved, because the InlineCost stuff is very much default policy-specific.

This seems like a cleaner approach than my current solution. I'll need to move the `EmitRemarks` over to the base class which looks to be okay given it can be defaulted to true. Thoughts?



================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:65
+void DefaultInlineAdvice::recordInliningWithCalleeDeletedImpl() {
+  if (EmitRemarks)
     emitInlinedInto(ORE, DLoc, Block, *Callee, *Caller, *OIC);
----------------
mtrofin wrote:
> Why add optional remark emission?
The SampleProfile.cpp inliner emits its own remarks via the `emitInlinedInto` interface. Without this you would get double remarks when dumping the SampleProfile.cpp inliner which I want to avoid. The rest of the comments suggests there's a better holistic solution so I'll add my thoughts to a below comment.


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

https://reviews.llvm.org/D94333