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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram added a comment.

Thanks for doing this! Much appreciated!



================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:774
   static StringRef getCanonicalFnName(StringRef FnName, StringRef Attr = "") {
-    static const char *knownSuffixes[] = { ".llvm.", ".part." };
+    static const char *knownSuffixes[] = {".llvm.", ".part.", ".__uniq."};
     if (Attr == "" || Attr == "all") {
----------------
As an aside, where does ".part." come from?


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:784
+        if (Suffix == ".__uniq." && FunctionSamples::HasUniqSuffix)
+          continue;
         auto It = Cand.rfind(Suffix);
----------------
```
Do we need a test where "llvm", "part" and "__uniq" appear in the symbol name?  Also, isn't it "__part"?
```


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:458
 
+  /// Return whether any name in the profile contains ".uniq." suffix.
+  virtual bool hasUniqSuffix() { return false; }
----------------
```
typo: "__uniq"
```


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:38
+bool FunctionSamples::UseMD5 = false;
+bool FunctionSamples::HasUniqSuffix = true;
 } // namespace sampleprof
----------------
Why is this true by default?


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:251
+    } else {
+      CalleeName = getCanonicalFnName(CalleeName, "selected");
+    }
----------------
Could you also add a code comment on how this default could be wrong if at all and its implications?


================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:211
+  for (const auto &I : NameTable) {
+    if (I.first.find(".__uniq.") != StringRef::npos) {
+      addSectionFlag(SecNameTable, SecNameTableFlags::SecFlagUniqSuffix);
----------------
```
Another suggestion to store __uniq as a constant somewhere and reuse that .
```


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:212
+  }
+
   ContextTrieNode *CalleeContext = getCalleeContextFor(DIL, CalleeName);
----------------
Saw this code snippet for the second time, also used in SampleProf.cpp. Could we refactor to one function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96932