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

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:762
 
-    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))
-          return EC;
-      }
-    } else if (ProfileIsCS) {
+    if (ProfileIsCS) {
       // Compute the ordered set of names, so we can
----------------
hoy wrote:
> wmi wrote:
> > Since Function in non context profile can also be represented using SampleContext, can we merge the case for cs and non-cs profile cases here?
> The processing of cs profile is a bit different from non-cs in that cs profile needs to be loaded in some order that profiles should be loaded for not only functions in the current module, but also for the callee functions out of current module. Note that `OrderedContexts` used only in the cs case diverges from non-cs case. I'm also changing how to load profiles for cs profiles in another patch which makes them diverged even more.
Ok, if we can come up with a solution to differentiate them in a query function or shrink the guarded range of ProfileIsCS, that will be ideal. I thought a bit but didn't get it right.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:782-789
+        if (useMD5()) {
+          if (!FuncGuidsToUse.count(std::stoull(FuncName.data())))
+            continue;
+        } else {
+          if (!FuncsToUse.count(FuncName) &&
+              (!Remapper || !Remapper->exist(FuncName)))
+            continue;
----------------
hoy wrote:
> wmi wrote:
> > It can be collapsed into one if. 
> > if (useMD() && !FuncGuidsToUse.count(std::stoull(FuncName.data())) ||
> >      (!FuncsToUse.count(FuncName) &&
> >               (!Remapper || !Remapper->exist(FuncName))))
> >   continue;
> Currently in both Md5 and string case, `FuncsToUse` contains the function real names computed by `FunctionSamples::getCanonicalFnName`. We could change that to include md5 names for the md5 case to unify the two paths. However, the md5 name string for a function needs to be stored somewhere before added to `FuncsToUse` which only takes a stringRef. That seems a bit complicated. What do you think?
Ok, then can we have
```
if (useMD() && !FuncGuidsToUse.count(std::stoull(FuncName.data())) ||
    (!useMD() && !FuncsToUse.count(FuncName) &&
            (!Remapper || !Remapper->exist(FuncName))))
continue;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108342