[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/lib/ProfileData/SampleProfWriter.cpp:116
+    SecType Type, uint32_t LayoutIdx, uint64_t SectionStart) {
+  const auto &Entry = SectionHdrLayout[LayoutIdx];
+  assert(Entry.Type == Type && "Unexpected section type");
----------------
hoy wrote:
> May be worth adding a range check here to make sure `LayoutIdx` is not out-of-range?
Good point. Assertion added. 


================
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:
> hoy wrote:
> > wmi wrote:
> > > 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. 
> > I see. A pre-determined index is probably the easiest way to go. I see the assert in `writeOneSection`. Perhaps adding more checks there, like a range check, to make sure the index value passed in here is good?
> Can you please also add a comment here on how the indices are determined? 
Added comment to describe how indices are determined.


================
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;
----------------
wmi wrote:
> hoy wrote:
> > hoy wrote:
> > > wmi wrote:
> > > > 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. 
> > > I see. A pre-determined index is probably the easiest way to go. I see the assert in `writeOneSection`. Perhaps adding more checks there, like a range check, to make sure the index value passed in here is good?
> > Can you please also add a comment here on how the indices are determined? 
> Added comment to describe how indices are determined.
assertion added. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D93254