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

[Hongtao Yu via Phabricator via llvm-commits]

hoy marked 3 inline comments as done.
hoy added a comment.

In D108342#2971668 <https://reviews.llvm.org/D108342#2971668>, @wenlei wrote:

>> 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?

Yes, that's the way we were using previously. There is an optimization we can do before it comes to the writer for pseudo probe. Pseudo probe comes with GUID so we can use that inside in the profile generator and unwinder. I think @wlei had a change for that which we can land separately.



================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:1049
     return EC;
+  FunctionSamples::UseMD5 = true;
   MD5StringBuf = std::make_unique<std::vector<std::string>>();
----------------
wenlei wrote:
> Can we move `FunctionSamples::UseMD5 = useMD5()` to be before `readImpl()` inside `read()` and then avoid this extra UseMD5 setting?
Actually `useMD5` relies on `readImpl() ` to be set. It is until the reader actually reads the SecNameTable we know we are reading a md5 profile or not.  The reader checks the SecFlagMD5Name flag to see if SecNameTable is filled with real names or md5 names.




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


================
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) {}
----------------
wenlei wrote:
> 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.  
md5 is not enabled with llvm-profgen for now. Will a test when it is enabled. 

It also depends on how we achieve md5 profile generation. If we rely on the writer to convert real names to md5, then CSPreliner should see real names here. Otherwise, it can see md5 codes and we need to build a guid map for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108342