[PATCH] D108342: [CSSPGO] Enable loading MD5 CS profile.

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

> The work on llvm-profgen side is not included. Currently I'm relying on llvm-profdata to get a md5 cs profile.

Is there blocker for this to be done in llvm-profgen? I imagine if llvm-profdata can generate md5 cs profile, llvm-profgen should be no different by reusing the writer to handle this?



================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:813
 
-    if (useMD5()) {
-      for (auto Name : FuncsToUse) {
-        auto GUID = std::to_string(MD5Hash(Name));
-        auto iter = FuncOffsetTable.find(StringRef(GUID));
-        if (iter == FuncOffsetTable.end())
-          continue;
-        const uint8_t *FuncProfileAddr = Start + iter->second;
-        assert(FuncProfileAddr < End && "out of LBRProfile section");
-        if (std::error_code EC =
-                readFuncProfile(FuncProfileAddr, FunctionSamples::ProfileIsCS))
-          return EC;
-      }
-    } else if (FunctionSamples::ProfileIsCS) {
+    if (FunctionSamples::ProfileIsCS) {
       // Compute the ordered set of names, so we can
----------------
As we discussed, use SampleProfileReader::ProfileIsCS instead.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:817
       // iterating through the ordered names.
       std::set<SampleContext> OrderedNames;
       for (auto Name : FuncOffsetTable) {
----------------
OrderedNames -> OrderedContexts


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:1049
     return EC;
+  FunctionSamples::UseMD5 = true;
   MD5StringBuf = std::make_unique<std::vector<std::string>>();
----------------
Can we move `FunctionSamples::UseMD5 = useMD5()` to be before `readImpl()` inside `read()` and then avoid this extra UseMD5 setting?


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:475
+      getRepInFormat(Location.second, FunctionSamples::UseMD5, FGUID);
+      MD5Names.push_back(FGUID);
+      Location.second = MD5Names.back();
----------------
Remove copies:
```
MD5Names.emplace_back();
getRepInFormat(Location.second, FunctionSamples::UseMD5, MD5Names.back());
```


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:985
     if (!Func || Func->isDeclaration())
-      InlinedGUIDs.insert(FunctionSamples::getGUID(Name));
 
----------------
Good catch here and below.


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:35
                            uint64_t ColdThreshold)
-    : ContextTracker(Profiles), ProfileMap(Profiles),
+    : ContextTracker(Profiles, nullptr), ProfileMap(Profiles),
       HotCountThreshold(HotThreshold), ColdCountThreshold(ColdThreshold) {}
----------------
I think this would break preinliner when building profiled call graph - ContextTracker.getFuncNameFor would fail. 

Add a test case for md5 pre-inliner? perhaps just add md5 version for an existing test case.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108342