[PATCH] D110658: [InlineAdvisor] Add -inline-replay-strict to replay inline decisions only in callers that have remarks

[Di Mo via Phabricator via llvm-commits]

modimo added a comment.

In D110658#3058700 <https://reviews.llvm.org/D110658#3058700>, @wenlei wrote:

> In D110658#3057147 <https://reviews.llvm.org/D110658#3057147>, @modimo wrote:
>
>> In D110658#3035652 <https://reviews.llvm.org/D110658#3035652>, @wenlei wrote:
>>
>>> Thanks for working on this. Though perhaps there's some misunderstanding on what "strict" mode means (sorry I should have clarified more beforehand). In the original inline replay patch (https://reviews.llvm.org/D83743), I mentioned strict/positive/negative mode.
>>>
>>> The only mode we have before and after this patch is the strict mode. "strict" means existing remarks indicate positive decision, and the rest are all considered negative decision. "positive" means existing remarks indicate positive decision, but the rest is left to other heuristics (or fall back advisors). Accordingly, negative means the input only specifies negative decision.
>>>
>>> The change is limiting the scope of replay to function, as opposed to module or whole program. A powerful inline replay framework would support scope={module|function} x mode={strict|positive|negative}.
>>>
>>> So I would suggest adding a flag `-inline-replay-scope=[function|module]` (with a default value), instead of `-inline-replay-strict`. Later on if we add positive/negative mode, we could add flag `-inline-replay-mode=[strict|positive|negative]`, but for now we don't need it. Thoughts?
>>
>> I think using scope to capture the functionality here makes sense. For your definition of strict I see it as modifier to positive/negative. A strict+negative replay would not inline the remarks present but will attempt to inline everything else and strict+positive would inline the remarks but not inline anything else. Non-strict+negative would make sure the remarks are not inlined but defer the rest of the decisions to the original advisor and non-strict+positive will inline the remarks but defer the rest to the original advisor.
>>
>> Strict+negative is useful because one way we can capture replay is by looking at all call-sites that remain in the assembly and eliminate all the rest (i.e. making sure they're inlined) which has the advantage that inlined sites no longer present in the assembly (e.g. simple accessors whose bodies completely get folded) will properly be inlined while in strict+positive mode because the dwarf information is gone no remarks are generated and the original call remains.
>
> Using strict as modifier would make it even more flexible. I didn't think strict+negative would be useful, so didn't give it much thought. Actually it's a bit weird to represent negative decision using the same remark format (`...` inlined into `...`) but relying on flag to tell positive vs negative, perhaps negative-ness can be represented with the remark line using `...` not inlined into `...` (same as missed remarks). This eliminates the need to tell positive/negative from command line and also enable mixing of positive and negative within a file (for non-strict mode). But we can defer that discussion/decision.

Looks like the remarks format for not inlining is "will not be inlined into" (see OptimizationRemarkMissed in Inliner.cpp) which can be used to distinguish negative decisions. That being said, negative mode itself is different in that it assumes all other legal sites will be inlined. Remark format is for when replay has a decision, mode format is what happens to all the other inline sites.

Negative mode only really works in function-scope since module scope would likely lead to never-ending compilation. In my testing, however, it has an opportunity to be more faithful to replay a full inline tree than positive/strict since the outcome is directly going after having identical call-sites at the end. Positive can deviate since non-matching sites get heuristics applied and strict loses information if no assembly instructions remain detailing that an inlining has occurred. Negative OTOH will leave all call sites that existed earlier intact while generally ensuring every other call site gets inlined since the call didn't exist in inlining that generated the replay. Regardless, separate discussion for separate patch.



================
Comment at: llvm/include/llvm/Analysis/ReplayInlineAdvisor.h:43
+  StringMap<bool> InlineSitesFromRemarks;
+  StringSet<> InlineCallersFromRemarks;
 };
----------------
wenlei wrote:
> modimo wrote:
> > wenlei wrote:
> > > nit: `InlineCallersFromRemarks` -> `InlinerFromRemarks`
> > This is to indicate which callers should have replay enforced in function scope, don't think `InlinerFromRemarks` is enough to capture that.
> What I meant is `Inliner` is equivalent to `InlineCaller`, but more of a concise and canonical term. I guess you intend to emphasize on top level inliner, but InlinerCaller doesn't really carry that info either. Not a big deal though.
Hmm maybe `CallersToReplay`? The key point is that this is a list of callers to replay on.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:103
+
+static cl::opt<std::string> CGSCCInlineReplayScope(
+    "cgscc-inline-replay-scope", cl::init(""),
----------------
wenlei wrote:
> Please use enum directly to avoid parsing strings and error checking in ReplayInlineAdvisor's ctor. Search for `cl::opt<SampleProfileFormat> OutputFormat` or `cl::opt<enum PassDebugLevel>PassDebugging`. Description needs to be updated to be per value too.
Ah nice, I was looking for functionality like that but couldn't find it. Changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110658