[PATCH] D68601: [SampleFDO] Add indexing for function profiles so they can be loaded on demand in ExtBinary format

[Wei Mi via Phabricator via llvm-commits]

wmi marked 2 inline comments as done.
wmi added a comment.

In D68601#1699113 <https://reviews.llvm.org/D68601#1699113>, @wenlei wrote:

> Thanks for making indexing available for extended binary format. We've recently adding indexing to binary format as well in an internal patch, and also observed similar (~30%) build time reduction for some large services.


Thanks for providing the data!

> We also have have the need to differentiate dead/cold symbols vs new symbols, so symbol list in extended binary format is useful to us as well - will try it out. (I noticed that a small change in profile generation tool (the equivalent of https://github.com/google/autofdo) is needed to populate that list though, it'd be nice if these are all part of LLVM)

You can use llvm-profdata to attach the profile symbol list section. Symbol list can be provided to llvm-profdata in a plain text file.

> Left some comments inline. In addition, I think `llvm-profdata/Inputs/sample-profile.proftext` need to be updated as well to include the new section for `roundtrip.test`

The roundtrip test doesn't contain any golden file in extbinary format. It generates the extbinary format profile on the fly. The input sample-profile.proftext doesn't need change.



================
Comment at: llvm/include/llvm/ProfileData/SampleProfWriter.h:206
   virtual void initSectionLayout() override {
     SectionLayout = {{SecProfSummary, 0, 0, 0},
                      {SecNameTable, 0, 0, 0},
----------------
wenlei wrote:
> With the addition of offset table at the end of sections but in the middle of section header, I found the name SectionLayout a bit confusing. This array actually represents the layout/order of section header (e.g. the reader order), but not the section (payload) layout (the writer order). I guess renaming it SectionHdrLayout or something alike may help..
> 
> Besides, some comments explaining why SecFuncOffsetTable need to be in the middle could help too, it looks like a hidden trick until I saw the comments in SampleProfileReaderExtBinaryBase::getFileSize().
Indeed SectionHdrLayout is a better name.  I changed it and added comment to explain why SecFuncOffsetTable is after SecLBRProfile in the profile but is before SecLBRProfile in the section header table. 


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:547-548
+      continue;
+    Data = Start + iter->second;
+    if (std::error_code EC = readFuncProfile())
+      return EC;
----------------
wenlei wrote:
> nit: similar to line 539, we could assert the random access here never attempts to touch anything beyond Size? 
assertion added.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68601