[PATCH] D148868: [llvm-profdata] ProfileReader cleanup - preparation for MD5 refactoring

[Wenlei He via Phabricator via llvm-commits]

wenlei added subscribers: wlei, hoy.
wenlei added a comment.

Hoisting all the MD5 stuff out of compact binary format, and merge them with extended binary format makes sense. Initially compact binary format was the only one using MD5, but later on Wei added MD5 support in extended binary as well, which caused some of the duplication this patch is trying to address.

However, from design POV, it's a good practice to keep the specific implementation down to leaf type as much as possible. Here MD5 only comes into play for extended binary and compact binary, so exposing that to all binary format is less than ideal. I think the problem is that compact binary and extended binary started out to be very different, but involved into something similar, so the type structure no longer reflects that commonality. Now in order to deduplicate code, you have to expose MD5 stuff to their lowest common ancestor, which is binary format.

Actually in https://reviews.llvm.org/D76255, we talked about eventually remove compact binary. Can we simply remove compact binary now? Then we can keep MD5 stuff in extended binary still, and there will be no duplicates.

cc @hoy @wlei



================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:571
+
+  /// Whether the profile uses MD5 for Sample Conexts for function names. This
+  /// can be one-way overriden by the user to force use MD5.
----------------
typo: Conexts->Contexts, for->or


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148868