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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:1049
     return EC;
+  FunctionSamples::UseMD5 = true;
   MD5StringBuf = std::make_unique<std::vector<std::string>>();
----------------
hoy wrote:
> 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.
> 
> 
Ok.. Taking another look, where in reader do we depend on `FunctionSamples::UseMD5` though? With the current definition (pasted below) for useMD5(), we could check useMD5() within reader, then we avoid this special case here?

Similar to ProfileIsCS, in general it's more consistent to use reader flag while we're in reader. 

```
bool useMD5() override { return MD5StringBuf.get(); }
```


================
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) {}
----------------
hoy wrote:
> 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.
Sounds good, add a TODO here then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108342