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

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
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;
----------------
wenlei wrote:
> 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.
Ah, I see. markContextSamplesInlined mark those contexts which will be inlined. For the rest, they are non-inlined and will be merged when getBaseSamplesFor is called. 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99146