[PATCH] D112948: [SamplePGO] Fix callsite sample lookup to use dwarf names when dwarf linkage name isn't available.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:253-254
+      Name = PrevDIL->getScope()->getSubprogram()->getName();
     S.push_back(
-        std::make_pair(LineLocation(getOffset(DIL), Discriminator),
-                       PrevDIL->getScope()->getSubprogram()->getLinkageName()));
+        std::make_pair(FunctionSamples::getCallSiteIdentifier(DIL), Name));
     PrevDIL = DIL;
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > What I meant is - can we just do this for now and avoid changes in getCallSiteIdentifier? If we want to fix FSAFDO, we can do it in a separate patch. 
> > > 
> > > ```
> > > 
> > > // Use C++ linkage name if possible.
> > > StringRef Name = PrevDIL->getScope()->getSubprogram()->getLinkageName();
> > > if (Name.empty())
> > >     Name = PrevDIL->getScope()->getSubprogram()->getName();
> > > 
> > > S.push_back(
> > >         std::make_pair(LineLocation(getOffset(DIL), Discriminator), Name));
> > > ```
> > This looks good for this change. I was saying the getCallSiteIdentifier change is still needed to make probe-based nested profile work since the currently it only deals with line numbers in this function.
> > 
> > 
> Are you saying change for getCallSiteIdentifier is needed for the linkage name issue with nested probe profile? But isn't there no change for `FunctionSamples::ProfileIsProbeBased` anyways inside `getCallSiteIdentifier`? Sorry I'm confused - I thought this patch is just for fixing linkage name issue as the description says, which is unrelated to nested probe profile.
It's not needed for the linkage name issue. I'll make it a separate change to target probe-based nested profile. Sorry for the confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112948