[PATCH] D147740: [llvm-profdata] Refactoring Sample Profile Reader to increase FDO build speed using MD5 as key to Sample Profile map

[William Junda Huang via Phabricator via llvm-commits]

huangjd marked 3 inline comments as done.
huangjd added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1319
+
+  mapped_type &operator[](const SampleContext &Ctx) {
+    return try_emplace(Ctx, FunctionSamples()).first->second;
----------------
davidxl wrote:
> This interface is confusing.  User expect it to return existing entry, but here it erases it. Should this interface be hidden (and not allowed with assert)?
In C++, [] is same as try_emplace with default constructed mapped_type, so I am keeping the behavior consistent. Keeping this function because many places use it. 


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1327
+    auto Ret = try_emplace(Ctx, FunctionSamples());
+    if (Ret.second)
+      Ret.first->second.setContext(Ctx);
----------------
davidxl wrote:
> is this always true as currently implemented?
This returns false if the existing entry actually has the same context, which indicates a match, rather than a MD5 collision, so no need to set the context again (And setting the context actually erase the flags in the context, which is not used for equality comparison or hasing) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147740