[PseudoProbe] Add PseudoProbeDescUpdatePass #99839

HaohaiWen · 2024-07-22T05:39:25Z

[PseudoProbe] Add PseudoProbeDescUpdatePass

Issue: #98127
Add PseudoProbeDescUpdatePass to update pseudo_probe_desc metadata for
functions generated by optimization transformation. Those functions may
contain pseudo probe discriminator so that llvm-profgen will try to get
its function name based on GUID and then crash.

Add PseudoProbeDescUpdatePass to update pseudo_probe_desc metadata for functions generated by optimization transformation. Those functions may contain pseudo probe discriminator so that llvm-profgen will try to get its function name based on GUID and then crash.

HaohaiWen · 2024-07-22T05:39:41Z

Issue: #98127

llvmbot · 2024-07-22T05:39:53Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-pgo

Author: Haohai Wen (HaohaiWen)

Changes

[PseudoProbe] Add PseudoProbeDescUpdatePass

Add PseudoProbeDescUpdatePass to update pseudo_probe_desc metadata for
functions generated by optimization transformation. Those functions may
contain pseudo probe discriminator so that llvm-profgen will try to get
its function name based on GUID and then crash.

Full diff: https://github.com/llvm/llvm-project/pull/99839.diff

5 Files Affected:

(modified) llvm/include/llvm/Transforms/IPO/SampleProfileProbe.h (+8)
(modified) llvm/lib/Passes/PassBuilderPipelines.cpp (+12-6)
(modified) llvm/lib/Passes/PassRegistry.def (+1)
(modified) llvm/lib/Transforms/IPO/SampleProfileProbe.cpp (+36-3)
(added) llvm/test/Transforms/SampleProfile/pseudo-probe-desc-update.ll (+35)

diff --git a/llvm/include/llvm/Transforms/IPO/SampleProfileProbe.h b/llvm/include/llvm/Transforms/IPO/SampleProfileProbe.h
index b52ef847d9db6..c15da7589dc7e 100644
--- a/llvm/include/llvm/Transforms/IPO/SampleProfileProbe.h
+++ b/llvm/include/llvm/Transforms/IPO/SampleProfileProbe.h
@@ -140,5 +140,13 @@ class PseudoProbeUpdatePass : public PassInfoMixin<PseudoProbeUpdatePass> {
   PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM);
 };
 
+// Update pseudo_probe_desc metadata for functions generated by optimization
+// transformation.
+class PseudoProbeDescUpdatePass
+    : public PassInfoMixin<PseudoProbeDescUpdatePass> {
+public:
+  PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM);
+};
+
 } // end namespace llvm
 #endif // LLVM_TRANSFORMS_IPO_SAMPLEPROFILEPROBE_H
diff --git a/llvm/lib/Passes/PassBuilderPipelines.cpp b/llvm/lib/Passes/PassBuilderPipelines.cpp
index 4fd5ee1946bb7..464379a5c638b 100644
--- a/llvm/lib/Passes/PassBuilderPipelines.cpp
+++ b/llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -1581,9 +1581,12 @@ PassBuilder::buildPerModuleDefaultPipeline(OptimizationLevel Level,
   // Now add the optimization pipeline.
   MPM.addPass(buildModuleOptimizationPipeline(Level, LTOPhase));
 
-  if (PGOOpt && PGOOpt->PseudoProbeForProfiling &&
-      PGOOpt->Action == PGOOptions::SampleUse)
-    MPM.addPass(PseudoProbeUpdatePass());
+  if (PGOOpt && PGOOpt->PseudoProbeForProfiling) {
+    if (PGOOpt->Action == PGOOptions::SampleUse)
+      MPM.addPass(PseudoProbeUpdatePass());
+    else
+      MPM.addPass(PseudoProbeDescUpdatePass());
+  }
 
   // Emit annotation remarks.
   addAnnotationRemarksPass(MPM);
@@ -1651,9 +1654,12 @@ PassBuilder::buildThinLTOPreLinkDefaultPipeline(OptimizationLevel Level) {
   if (RunPartialInlining)
     MPM.addPass(PartialInlinerPass());
 
-  if (PGOOpt && PGOOpt->PseudoProbeForProfiling &&
-      PGOOpt->Action == PGOOptions::SampleUse)
-    MPM.addPass(PseudoProbeUpdatePass());
+  if (PGOOpt && PGOOpt->PseudoProbeForProfiling) {
+    if (PGOOpt->Action == PGOOptions::SampleUse)
+      MPM.addPass(PseudoProbeUpdatePass());
+    else
+      MPM.addPass(PseudoProbeDescUpdatePass());
+  }
 
   // Handle Optimizer{Early,Last}EPCallbacks added by clang on PreLink. Actual
   // optimization is going to be done in PostLink stage, but clang can't add
diff --git a/llvm/lib/Passes/PassRegistry.def b/llvm/lib/Passes/PassRegistry.def
index 3b92823cd283b..6116785f580a7 100644
--- a/llvm/lib/Passes/PassRegistry.def
+++ b/llvm/lib/Passes/PassRegistry.def
@@ -119,6 +119,7 @@ MODULE_PASS("print<inline-advisor>", InlineAdvisorAnalysisPrinterPass(dbgs()))
 MODULE_PASS("print<module-debuginfo>", ModuleDebugInfoPrinterPass(dbgs()))
 MODULE_PASS("pseudo-probe", SampleProfileProbePass(TM))
 MODULE_PASS("pseudo-probe-update", PseudoProbeUpdatePass())
+MODULE_PASS("pseudo-probe-desc-update", PseudoProbeDescUpdatePass())
 MODULE_PASS("recompute-globalsaa", RecomputeGlobalsAAPass())
 MODULE_PASS("rel-lookup-table-converter", RelLookupTableConverterPass())
 MODULE_PASS("rewrite-statepoints-for-gc", RewriteStatepointsForGC())
diff --git a/llvm/lib/Transforms/IPO/SampleProfileProbe.cpp b/llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
index b489d4fdaa210..e83ca33b2c813 100644
--- a/llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
+++ b/llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
@@ -341,9 +341,7 @@ uint32_t SampleProfileProber::getCallsiteId(const Instruction *Call) const {
   return Iter == CallProbeIds.end() ? 0 : Iter->second;
 }
 
-void SampleProfileProber::instrumentOneFunc(Function &F, TargetMachine *TM) {
-  Module *M = F.getParent();
-  MDBuilder MDB(F.getContext());
+static StringRef getFuncName(const Function &F) {
   // Since the GUID from probe desc and inline stack are computed separately, we
   // need to make sure their names are consistent, so here also use the name
   // from debug info.
@@ -353,6 +351,13 @@ void SampleProfileProber::instrumentOneFunc(Function &F, TargetMachine *TM) {
     if (FName.empty())
       FName = SP->getName();
   }
+  return FName;
+}
+
+void SampleProfileProber::instrumentOneFunc(Function &F, TargetMachine *TM) {
+  Module *M = F.getParent();
+  MDBuilder MDB(F.getContext());
+  StringRef FName = getFuncName(F);
   uint64_t Guid = Function::getGUID(FName);
 
   // Assign an artificial debug line to a probe that doesn't come with a real
@@ -513,3 +518,31 @@ PreservedAnalyses PseudoProbeUpdatePass::run(Module &M,
   }
   return PreservedAnalyses::none();
 }
+
+PreservedAnalyses PseudoProbeDescUpdatePass::run(Module &M,
+                                                 ModuleAnalysisManager &AM) {
+  if (NamedMDNode *FuncInfo = M.getNamedMetadata(PseudoProbeDescMetadataName)) {
+    DenseSet<uint64_t> GuidSet;
+    for (const auto *Operand : FuncInfo->operands()) {
+      const auto *MD = cast<MDNode>(Operand);
+      auto *GUID = mdconst::dyn_extract<ConstantInt>(MD->getOperand(0));
+      GuidSet.insert(GUID->getZExtValue());
+    }
+
+    MDBuilder MDB(M.getContext());
+    for (Function &F : M) {
+      if (F.isDeclaration())
+        continue;
+      StringRef FName = getFuncName(F);
+      uint64_t Guid = Function::getGUID(FName);
+      if (GuidSet.insert(Guid).second) {
+        // Set those function's hash to 0 since it may vary depending on IR
+        // input to optimization transformation.
+        auto *MD = MDB.createPseudoProbeDesc(Guid, 0, FName);
+        FuncInfo->addOperand(MD);
+        LLVM_DEBUG(dbgs() << "New fixup pseudo_probe_desc MD: " << *MD << "\n");
+      }
+    }
+  }
+  return PreservedAnalyses::all();
+}
diff --git a/llvm/test/Transforms/SampleProfile/pseudo-probe-desc-update.ll b/llvm/test/Transforms/SampleProfile/pseudo-probe-desc-update.ll
new file mode 100644
index 0000000000000..4a114c5b11fb3
--- /dev/null
+++ b/llvm/test/Transforms/SampleProfile/pseudo-probe-desc-update.ll
@@ -0,0 +1,35 @@
+; RUN: opt < %s -passes='pseudo-probe-desc-update' -S | FileCheck %s
+
+; CHECK:      !llvm.pseudo_probe_desc = !{!0, !1, !2, !3, !4, !5}
+; CHECK:      !0 = !{i64 -2012135647395072713, i64 4294967295, !"bar"}
+; CHECK-NEXT: !1 = !{i64 9204417991963109735, i64 72617220756, !"work"}
+; CHECK-NEXT: !2 = !{i64 6699318081062747564, i64 844700110938769, !"foo"}
+; CHECK-NEXT: !3 = !{i64 -2624081020897602054, i64 281563657672557, !"main"}
+; CHECK-NEXT: !4 = !{i64 6028998432455395745, i64 0, !"extract1"}
+; CHECK-NEXT: !5 = !{i64 -8314581669044049530, i64 0, !"_Zextract2v"}
+
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @extract1() !dbg !1 {
+entry:
+  ret void
+}
+
+define void @extract2() !dbg !2 {
+entry:
+  ret void
+}
+
+!llvm.pseudo_probe_desc = !{!4, !5, !6, !7}
+!llvm.module.flags = !{!8, !9}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !3, emissionKind: FullDebug,  splitDebugInlining: false, debugInfoForProfiling: true)
+!1 = distinct !DISubprogram(name: "extract1", unit: !0)
+!2 = distinct !DISubprogram(name: "extract2", linkageName: "_Zextract2v", unit: !0)
+!3 = !DIFile(filename: "foo.c", directory: "/home/test")
+!4 = !{i64 -2012135647395072713, i64 4294967295, !"bar"}
+!5 = !{i64 9204417991963109735, i64 72617220756, !"work"}
+!6 = !{i64 6699318081062747564, i64 844700110938769, !"foo"}
+!7 = !{i64 -2624081020897602054, i64 281563657672557, !"main"}
+!8 = !{i32 7, !"Dwarf Version", i32 5}
+!9 = !{i32 2, !"Debug Info Version", i32 3}

WenleiHe

I'm not sure if this is the right fix. PGO generally operates with pre-split functions. So ideally we want all the funclets to map back to the same original functions as opposed to using separate functions to represent funclets.

Compiler generated functions are not unique to partial inline. Coroutine split is another example. How is that handled and how is partial inline different? cc @wlei-llvm

WenleiHe · 2024-07-22T06:11:55Z

llvm/lib/Passes/PassBuilderPipelines.cpp

+    if (PGOOpt->Action == PGOOptions::SampleUse)
+      MPM.addPass(PseudoProbeUpdatePass());
+    else
+      MPM.addPass(PseudoProbeDescUpdatePass());


Why is this not applicable to SampleUse path?

I think we don't use pseudo_probe_desc for SampleUse?

We do. PseudoProbeUpdatePass for pseudo-probe.

wlei-llvm

Do you have a small repro for the crash? I'm curious about what the missing function name looks like. For pseudo probe, it tries to use function name from debug info/dwarf to compute the GUID so that the symbol can be matched to the source code function, otherwise it's not meaningful to generate a function profile that compiler can't consume.
If this is only a rare case(like the debug info is missing for that function), we can probably just ignore it in llvm-profgen(add some stats/warning), or we may need to fix or maintain the function to use symbol from dwarf.

HaohaiWen · 2024-07-22T06:35:44Z

Do you have a small repro for the crash? I'm curious about what the missing function name looks like. For pseudo probe, it tries to use function name from debug info/dwarf to compute the GUID so that the symbol can be matched to the source code function, otherwise it's not meaningful to generate a function profile that compiler can't consume. If this is only a rare case(like the debug info is missing for that function), we can probably just ignore it in llvm-profgen(add some stats/warning), or we may need to fix or maintain the function to use symbol from dwarf.

It's cpu2017/500.perlbench when partialinliner is on.

PartialInliner creates a new function.
There's call inst with pseudo-probe discriminator in this function.
llvm/lib/CodeGen/PseudoProbeInserter.cpp generates a PSEUDO_PROBE MI which GUID was generated by using this new function's name.
Therefore a pseudo probe record will use this new function's GUID but there's not any corresponding item in .pseudo_probe_desc.
llvm-profgen crashed when calling ProbeDecoder.getFuncDescForGUID.

void ProfiledBinary::decodePseudoProbe(const ELFObjectFileBase *Obj) {
......
  // Build TopLevelProbeFrameMap to track size for optimized inlinees when probe
  // is available
  if (TrackFuncContextSize) {
    for (const auto &Child : ProbeDecoder.getDummyInlineRoot().getChildren()) {
      auto *Frame = Child.second.get();
      StringRef FuncName =
          ProbeDecoder.getFuncDescForGUID(Frame->Guid)->FuncName;
      TopLevelProbeFrameMap[FuncName] = Frame;
    }
  }

wlei-llvm · 2024-07-22T06:38:09Z

Compiler generated functions are not unique to partial inline. Coroutine split is another example. How is that handled and how is partial inline different? cc @wlei-llvm

We use debug info symbol so that the different suffix function can be mapped to the original function, it seems also applicable to partial inlining. but maybe the debug info is only missing for that function:

  if (auto *SP = F.getSubprogram()) {
    FName = SP->getLinkageName();
    if (FName.empty())
      FName = SP->getName();
  }

HaohaiWen · 2024-07-22T06:48:20Z

Compiler generated functions are not unique to partial inline. Coroutine split is another example. How is that handled and how is partial inline different? cc @wlei-llvm

We use debug info symbol so that the different suffix function can be mapped to the original function, it seems also applicable to partial inlining. but maybe the debug info is only missing for that function:
  if (auto *SP = F.getSubprogram()) {
    FName = SP->getLinkageName();
    if (FName.empty())
      FName = SP->getName();
  }

Lost of debug info should be fixed.
However, I think it shouldn't cause crash of llvm-profgen in long term.

WenleiHe · 2024-07-22T17:21:58Z

Compiler generated functions are not unique to partial inline. Coroutine split is another example. How is that handled and how is partial inline different? cc @wlei-llvm

We use debug info symbol so that the different suffix function can be mapped to the original function, it seems also applicable to partial inlining. but maybe the debug info is only missing for that function:
  if (auto *SP = F.getSubprogram()) {
    FName = SP->getLinkageName();
    if (FName.empty())
      FName = SP->getName();
  }
Lost of debug info should be fixed. However, I think it shouldn't cause crash of llvm-profgen in long term.

So, if missing debug info for partial inline funclet is the root case, we can deal with this by:

Fixing missing debug info for paritial inline.
Let llvm-profgen emit a warning such cases and tolerate it instead of crashing.

HaohaiWen · 2024-07-23T02:34:53Z

Compiler generated functions are not unique to partial inline. Coroutine split is another example. How is that handled and how is partial inline different? cc @wlei-llvm

We use debug info symbol so that the different suffix function can be mapped to the original function, it seems also applicable to partial inlining. but maybe the debug info is only missing for that function:
  if (auto *SP = F.getSubprogram()) {
    FName = SP->getLinkageName();
    if (FName.empty())
      FName = SP->getName();
  }
Lost of debug info should be fixed. However, I think it shouldn't cause crash of llvm-profgen in long term.
So, if missing debug info for partial inline funclet is the root case, we can deal with this by:

Fixing missing debug info for paritial inline.

Let llvm-profgen emit a warning such cases and tolerate it instead of crashing.

I think 2 is necessary and better to fix 1 since lost debug info is something always happened.
Will you fix llvm-profgen issue?
You can simply create a case by manually delete debug info or just let PseudoProbeInserter ignore linkagename.

WenleiHe · 2024-07-23T05:05:00Z

I think 2 is necessary and better to fix 1 since lost debug info is something always happened.

Why is this something that always happened? This is about an entire function missing debug info. Evidently, we haven't run into any such cases in any of our code base. Fixing #1 is handling the root cause, while fixing #2 is just hiding the underlying issue.

Will you fix llvm-profgen issue?

We will get to that, feel free to send a patch too. But fixing root cause (#1) should be prioritized over this.

HaohaiWen · 2024-07-23T05:52:04Z

I think 2 is necessary and better to fix 1 since lost debug info is something always happened.

Why is this something that always happened? This is about an entire function missing debug info. Evidently, we haven't run into any such cases in any of our code base. Fixing #1 is handling the root cause, while fixing #2 is just hiding the underlying issue.

Will you fix llvm-profgen issue?

We will get to that, feel free to send a patch too. But fixing root cause (#1) should be prioritized over this.

I had fixed several debug info missing issues. I believe there must be other places.
Anyway, adding a warning in llvm-profgen that hint user we have lost some debuginfo is more friendly than raising exception about GUID.

tianqingw · 2024-07-24T08:50:42Z

Regarding debug info, with current implementation, I doubt we can map the debug info after PartialInline to the original function: #100327

llvmbot added PGO Profile Guided Optimizations llvm:transforms labels Jul 22, 2024

HaohaiWen requested review from WenleiHe, htyu and williamweixiao July 22, 2024 05:40

WenleiHe requested a review from wlei-llvm July 22, 2024 05:47

WenleiHe requested changes Jul 22, 2024

View reviewed changes

wlei-llvm reviewed Jul 22, 2024

View reviewed changes

HaohaiWen closed this Apr 21, 2025

HaohaiWen deleted the pseudoprobe branch June 24, 2025 06:24

[PseudoProbe] Add PseudoProbeDescUpdatePass #99839

[PseudoProbe] Add PseudoProbeDescUpdatePass #99839

Uh oh!

Conversation

HaohaiWen commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HaohaiWen commented Jul 22, 2024

Uh oh!

llvmbot commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WenleiHe left a comment

Choose a reason for hiding this comment

Uh oh!

WenleiHe Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

HaohaiWen Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

WenleiHe Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

wlei-llvm left a comment

Choose a reason for hiding this comment

Uh oh!

HaohaiWen commented Jul 22, 2024

Uh oh!

wlei-llvm commented Jul 22, 2024

Uh oh!

HaohaiWen commented Jul 22, 2024

Uh oh!

WenleiHe commented Jul 22, 2024

Uh oh!

HaohaiWen commented Jul 23, 2024

Uh oh!

WenleiHe commented Jul 23, 2024

Uh oh!

HaohaiWen commented Jul 23, 2024

Uh oh!

tianqingw commented Jul 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HaohaiWen commented Jul 22, 2024 •

edited

Loading

llvmbot commented Jul 22, 2024 •

edited

Loading