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

Di Mo via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 11 23:18:23 PDT 2021


modimo added a comment.

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.



================
Comment at: llvm/include/llvm/Analysis/ReplayInlineAdvisor.h:43
+  StringMap<bool> InlineSitesFromRemarks;
+  StringSet<> InlineCallersFromRemarks;
 };
----------------
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.


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:50
 
+cl::opt<bool> InlineReplayStrict(
+    "inline-replay-strict", cl::init(false),
----------------
wenlei wrote:
> Sample loader replay was independent of cgscc replay as they use different switches for taking different inputs. Now the "mode" switch is global, so both replays are tied to a single mode even if they use different input files. I think it's more flexible if they are still kept independent (the hot/cold thresholds etc are all kept separate for the two inliners). 
Makes sense, split


================
Comment at: llvm/lib/Analysis/ReplayInlineAdvisor.cpp:53
+                           .first.drop_front();
     auto CallSite = Pair.second.split(";").first;
 
----------------
wenlei wrote:
> `Pair.second.drop_back()` works? or you expect something after `;`?
If the remarks are generated via -Rpass there can be more after:

```
inline.cpp:8:12: remark: _Z3foov inlined into main with (cost=0, threshold=375) at callsite main:2:12; [-Rpass=inline]
    return foo();
```


================
Comment at: llvm/lib/Analysis/ReplayInlineAdvisor.cpp:69-72
+    for (const auto &Remark : InlineSitesFromRemarks)
+      if (!Remark.second)
+        LLVM_DEBUG(dbgs() << "Inline Replay Strict: Did not apply "
+                          << Remark.first() << "\n");
----------------
wenlei wrote:
> I'm not sure whether printing such debug dumps from dtor is a good idea. Lifetime of objects could potentially be somewhat arbitrary, but logs better be printed in more controlled/organized fashion. Looking at the test case, it also seems like these prints are added for testing purpose? See the comments in the test case - we better avoid using debug prints for testing. 
> 
> 
This was originally intended as a way to easily check which remark was not processed to give a hint to the user that an unexpected inlining difference could be caused by it. In practice I haven't found it to be too useful because other reasons more often led to replay mismatching:

1. Source line mismatch between GCC and LLVM, seen on destructor placement
2. Pre-inline IR difference, GCC performs tail recursion elimination pre-inline which changes what call-sites remain post-inline compared to Clang.

So these end up being red herrings. Removed.


================
Comment at: llvm/lib/Analysis/ReplayInlineAdvisor.cpp:99
+
+  if (Strict) {
+    if (InlineCallersFromRemarks.count(CB.getFunction()->getName()))
----------------
wenlei wrote:
> nit:
> 
> ```
> if (!Strict || InlineCallersFromRemarks.count(CB.getFunction()->getName())) {
>   ...
> }
> else if (Strict) {
>   if (OriginalAdvisor)
>     return OriginalAdvisor->getAdvice(CB);
>   return {};
> }
> ```
> 
> This way we don't need a lambda - since it's only called once it can be inlined.
I like it, changed.


================
Comment at: llvm/test/Transforms/Inline/cgscc-inline-replay.ll:21
+; REPLAY-STRICT: '_Z3sumii' inlined into 'main' with (cost=always)
+; REPLAY-STRICT: Inline Replay Strict: Did not apply _Z3subii_Z3sumii:1:0 @ main:3:0.1
 
----------------
wenlei wrote:
> This would fail ninja check with release builds. Usually we try to avoid testing that requires debug builds unless it's critical and no other way to observe. 
Removed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110658



More information about the llvm-commits mailing list