[PATCH] D99146: [CSSPGO][llvm-profgen] Context-sensitive global pre-inliner

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:181
+    bool ShouldInline = false;
+    if ((ShouldInline = shouldInline(Candidate))) {
+      // We mark context as inined as the corresponding context profile
----------------
wenlei wrote:
> wmi wrote:
> > wmi wrote:
> > > The definition of ShouldInline is only used in LLVM_DEBUG. It may trigger warning in release mode.
> > I didn't find if the candidate should not be inlined, where the context profile is merged into the base profile. Could you show me?
> The merge is done within getBaseSamplesFor on-demand, same as how it's done in compiler.
Good point, let me check and adjust.


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:181-189
+    if ((ShouldInline = shouldInline(Candidate))) {
+      // We mark context as inined as the corresponding context profile
+      // won't be merged into that function's base profile.
+      ContextTracker.markContextSamplesInlined(Candidate.CalleeSamples);
+      Candidate.CalleeSamples->getContext().setAttribute(
+          ContextShouldBeInlined);
+      FuncFinalSize += Candidate.SizeCost;
----------------
wmi wrote:
> wmi wrote:
> > The definition of ShouldInline is only used in LLVM_DEBUG. It may trigger warning in release mode.
> I didn't find if the candidate should not be inlined, where the context profile is merged into the base profile. Could you show me?
The merge is done within getBaseSamplesFor on-demand, same as how it's done in compiler.


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:199
+    if (!CQueue.empty())
+      dbgs() << "  Inline candidates ignored due to size limit (inliner "
+                "original size: "
----------------
wmi wrote:
> Included in LLVM_DEBUG.
This is all in LLVM_DEBUG on line 197. Do you mean we need a separate LLVM_DEBUG? 


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:221
+
+  auto printProfileNames = [](StringMap<FunctionSamples> &Profiles,
+                              bool IsInput) {
----------------
wmi wrote:
> printProfileNames is only used in LLVM_DEBUG. Need to include it in #ifndef NDEBUG.
Good point, will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99146