[PATCH] D96932: [SampleFDO] Support enabling -funique-internal-linkage-name

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

Actually we are not canonicalizing the names in profile from compiler side, right? We are only canonicalizing names from IR to match names from profile.

Thinking about it more, doing canonicalization in compiler for CSSPGO may be trickier, because we try to reuse a single string for a context. It's feasible to canonicalize context strings in compiler, but it will have cost.

> One kind of regression is when we flip -funique-internal-linkage-name on, the profile is collected from binary built without -funique-internal-linkage-name so it has no uniq suffix, but the IR in the optimized build contains the suffix. This may introduce transient regression.

This could happen when we migrate. To tackle this one, SecFlagUniqSuffix added in this patch is good enough and we don't need to canonicalize names in profile from compiler side, as they can still be done in the profile generation tools.

> In rare case, user may also want to disable -funique-internal-linkage-name after it has been enabled for a while, to triage something for example. In that case, profile will contain the suffix but IR in the optimized build won't.

How is this handled currently? Also to me this feel not so important and it's more like debugging helper. If we don't intend to support such case, I don't see a need to move all name canonicalization from profile generation tool into the compiler..



================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:211
+  // seeing the flag in the profile.
+  for (const auto &I : NameTable) {
+    if (I.first.find(FunctionSamples::UniqSuffix) != StringRef::npos) {
----------------
Ideally, this flag should be derived from whether the binary was built with unique names. If we try to derive the flag from existing names, whether a profile happens to not contain static symbol (even if -funique-internal-linkage-name is always on) may affect how names are canonicalized for pgo pass2 (if we have new static symbols there)..

Not sure if it would happen in practice, but this is more from consistency angle rather than a practical concern..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96932