[llvm] a2768b4 - [CSSPGO] Honor preinliner decision for ThinLTO importing

[Wenlei He via llvm-commits]

Author: Wenlei He
Date: 2021-09-02T08:24:06-07:00
New Revision: a2768b4732a0216dfd346d34e428685f03f10549

URL: https://github.com/llvm/llvm-project/commit/a2768b4732a0216dfd346d34e428685f03f10549
DIFF: https://github.com/llvm/llvm-project/commit/a2768b4732a0216dfd346d34e428685f03f10549.diff

LOG: [CSSPGO] Honor preinliner decision for ThinLTO importing

When pre-inliner decision is used for CSSPGO, we should take that into account for ThinLTO importing as well, so post-link sample loader inliner can favor that decision. This is handled by a small tweak in this patch. It also includes a change to transfer preinliner decision when merging context.

Differential Revision: https://reviews.llvm.org/D109088

Added: 
    

Modified: 
    llvm/lib/Transforms/IPO/SampleContextTracker.cpp
    llvm/lib/Transforms/IPO/SampleProfile.cpp
    llvm/test/Transforms/SampleProfile/csspgo-use-preinliner.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/IPO/SampleContextTracker.cpp b/llvm/lib/Transforms/IPO/SampleContextTracker.cpp
index b644b0ce5d64b..2a9a13f2dafaf 100644
--- a/llvm/lib/Transforms/IPO/SampleContextTracker.cpp
+++ b/llvm/lib/Transforms/IPO/SampleContextTracker.cpp
@@ -545,6 +545,8 @@ void SampleContextTracker::mergeContextNode(ContextTrieNode &FromNode,
     ToSamples->merge(*FromSamples);
     ToSamples->getContext().setState(SyntheticContext);
     FromSamples->getContext().setState(MergedContext);
+    if (FromSamples->getContext().hasAttribute(ContextShouldBeInlined))
+      ToSamples->getContext().setAttribute(ContextShouldBeInlined);
   } else if (FromSamples) {
     // Transfer FromSamples from FromNode to ToNode
     ToNode.setFunctionSamples(FromSamples);

diff  --git a/llvm/lib/Transforms/IPO/SampleProfile.cpp b/llvm/lib/Transforms/IPO/SampleProfile.cpp
index 2b54c5981d5da..ba6f6d629f957 100644
--- a/llvm/lib/Transforms/IPO/SampleProfile.cpp
+++ b/llvm/lib/Transforms/IPO/SampleProfile.cpp
@@ -977,7 +977,14 @@ void SampleProfileLoader::findExternalInlineCandidate(
     // For CSSPGO profile, retrieve candidate profile by walking over the
     // trie built for context profile. Note that also take call targets
     // even if callee doesn't have a corresponding context profile.
-    if (!CalleeSample || CalleeSample->getEntrySamples() < Threshold)
+    if (!CalleeSample)
+      continue;
+
+    // If pre-inliner decision is used, honor that for importing as well.
+    bool PreInline =
+        UsePreInlinerDecision &&
+        CalleeSample->getContext().hasAttribute(ContextShouldBeInlined);
+    if (!PreInline && CalleeSample->getEntrySamples() < Threshold)
       continue;
 
     StringRef Name = CalleeSample->getFuncName();
@@ -1299,10 +1306,13 @@ SampleProfileLoader::shouldInlineCandidate(InlineCandidate &Candidate) {
   // we replay that inline decision under `sample-profile-use-preinliner`.
   // Note that we don't need to handle negative decision from preinliner as
   // context profile for not inlined calls are merged by preinliner already.
-  if (UsePreInlinerDecision &&
-      Candidate.CalleeSamples->getContext().hasAttribute(
-          ContextShouldBeInlined))
-    return InlineCost::getAlways("preinliner");
+  SampleContext &Context = Candidate.CalleeSamples->getContext();
+  if (UsePreInlinerDecision && Context.hasAttribute(ContextShouldBeInlined))
+    // Once two node are merged due to promotion, we're losing some context
+    // so the original context-sensitive preinliner decision should be ignored
+    // for SyntheticContext.
+    if (!Context.hasState(SyntheticContext))
+      return InlineCost::getAlways("preinliner");
 
   // For old FDO inliner, we inline the call site as long as cost is not
   // "Never". The cost-benefit check is done earlier.

diff  --git a/llvm/test/Transforms/SampleProfile/csspgo-use-preinliner.ll b/llvm/test/Transforms/SampleProfile/csspgo-use-preinliner.ll
index af3c70111ed05..1fb9a7972e3b5 100644
--- a/llvm/test/Transforms/SampleProfile/csspgo-use-preinliner.ll
+++ b/llvm/test/Transforms/SampleProfile/csspgo-use-preinliner.ll
@@ -8,9 +8,12 @@
 ; DEFAULT-NOT: inlined into
 
 ; PREINLINE-NOT: inlined into
+; `[main:3 @ _Z5funcAi]` does not have preinline decision, so it's up for loader inliner to decided.
 ; PREINLINE: '_Z5funcAi' inlined into 'main'
+; `[main:3 @ _Z5funcAi:1 @ _Z8funcLeafi]` is inlined according to preinline decision.
 ; PREINLINE: '_Z8funcLeafi' inlined into 'main'
-; PREINLINE: '_Z8funcLeafi' inlined into '_Z5funcBi'
+; Even though `[main:3.1 @ _Z5funcBi]` context is marked should inline, `_Z5funcBi` is a noinline function, so we honor that and don't inline.
+; When _Z5funcBi is promoted to be top level context-less profile, `[_Z5funcBi:1 @ _Z8funcLeafi]` becomes synthetic context, so preinline decision is ignored and we don't inline `_Z8funcLeafi`.
 ; PREINLINE-NOT: inlined into
 
 @factor = dso_local global i32 3, align 4, !dbg !0