[PATCH] D118390: [InstrProf] Make the IndexedInstrProf header backwards compatible.

Thu Jan 27 11:02:52 PST 2022

snehasish added a comment.

In D118390#3276782 <https://reviews.llvm.org/D118390#3276782>, @davidxl wrote:

> The refactoring looks good.  However in what situation do we need to modify the main header? There might be other ways to achieve the functionality.

We discussed some alternatives offline, summarizing here:

The motivation behind this change is the desire to add another (optional) ondiskhashtable to the indexed profile format. This will hold memory profile information which will be used to guide the memory allocator [1]. However the phase ordering of memory instrumentation (envisioned to be enabled at the same time as FDO instrumentation) implies that the current "function name & CFG hash" based indexing for the existing on disk hashtable is incompatible for memprof profile data. While it may be possible to refactor out the has computation to reuse, the approach is brittle and there are unknowns along with a larger radius of potentially affected users. Adding a new field to the header allows us provision another memprof profile data section. This section will contain metadata and an on disk hash table with the profile data.

Why not use the Unused field?
Existing profiles may break, at least the compat tests in the repo have old profiles which have this field set.

Why not add indirection to the existing on disk table header?
The on disk table structure allows for metadata before the actual payload and this approach is feasible. However, it makes the logic in IndexedInstrProfReader even more complicated and harder to follow.

I'll add some more static checks to ensure that users don't inadvertently break the ordering based computation here.

[1] https://groups.google.com/g/llvm-dev/c/h1DvHguLpxU/m/yGE-zyrhAAAJ

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118390