[PATCH] D89524: [NFC][SampleFDO] Move some common stuff from SampleProfileReaderExtBinary/SampleProfileWriterExtBinary to their parent classes.

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:206
+  switch (Type) {
+  case SecProfileSymbolList:
+    if (auto EC = writeProfileSymbolListSection())
----------------
hoy wrote:
> wmi wrote:
> > hoy wrote:
> > > Why is this separated from `writeOneSection`?
> > That is somewhat explained in the reply to Wenlei's mail. I want to put a section to an inheritance level when foreseeably new subclasses created in other inheritance branch will not use it. Surely whether SecProfileSymbolList will not be used by other new formats are questionable. I just use it as an example and it will be easy to move the section up.
> I see. Can I ask if you are going to introduce a new ExtBinary variant? Since we are also adding a new section (for pseudo probe) , I'm not sure if it should be treated as an optional section of ExtBinaryBase or a required section of a new variant. 
Yes, I am going to add a new ExtBinary variant. 

To introduce it a little bit, we have a synthesized profile which is merged from many sample profiles and we use it for targets without regular sample profile. The merged profile is quite large. Previously inline instances in the synthesized profile are all flattened, i.e., all inline instances in the profile are extracted and merged to their outline instance. We found selectively flattening instead of fully flattening the synthesized profile is helpful for performance, however it may increase compile time significantly in the postlink phase during profile loading. To solve that, we are trying to split the profile into two parts, one part containing inline instance and the other part not containing inline instance. When thinlto is used, the second profile loading pass in postlink phase will only have to load the part containing inline instance, which will minimize the compile time impact.

For the new variant we are going to propose, it doesn't need SecProfileSymbolList for the moment because the synthesized profile doesn't need SecProfileSymbolList. But we find in some initial experiment that we can selectively flatten regular sampleFDO profile without performance impact but with much smaller profile size, so the new variant could be possibly useful for regular sampleFDO too in the future. In that case, SecProfileSymbolList will be needed. 

With that being said, maybe SecProfileSymbolList is not a good example to be put in a subclass, we can move SecProfileSymbolList to the Base class if you agree. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D89524