[PATCH] D93254: [NFC][SampleFDO] Preparation to support multiple sections with the same type in ExtBinary format.

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfWriter.h:249
+    // current section.
+    uint32_t LayoutIndex;
+  };
----------------
hoy wrote:
> Can this be made a field of `SecHdrTableEntry`?
Good point. Done. 


================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:250
     const StringMap<FunctionSamples> &ProfileMap) {
-  if (auto EC = writeOneSection(SecProfSummary, ProfileMap))
+  if (auto EC = writeOneSection(SecProfSummary, 0, ProfileMap))
     return EC;
----------------
hoy wrote:
> wmi wrote:
> > hoy wrote:
> > > How are the constant indices determined? I'm wondering if it's possible to automatically assign indices to avoid collision.
> > The indice order should match the order in the SampleProfileWriterExtBinary::SectionHdrLayout. I find the indice order in the patch is not up-to-date. I will update the patch.
> > 
> > 
> > 
> I'm wondering what writing multiple same-typed sections will look like in future. Will `writeOneSection` take extra arguments? I'm thinking if we could still have a helper like `getEntryInLayout` to automatically retrieve layout index which may be helpful to adding new sections.
Right, it will be perfect to do it automatically. However currently if the profile contains multiple same-typed sections, we have no way to tell them apart without an index. The index of each section is in the same order as the section order in the profile section header table, but may be different from the order in which writeOneSection is called. That brings some difficulty to automatically assign the index.

An example is: writeOneSection for FuncOffsetTable has to be called after writeOneSection for LBRProfile (We are populating FuncOffsetTable while LBRProfile section is written out), but FuncOffsetTable has to be read before LBRProfile so we can read the function profiles which will actually be used. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D93254