[compiler-rt] [llvm] [InstrFDO]Allow indexed profile reader to parse compatible future versions and returns errors for incompatible ones. (PR #88212)

Kazu Hirata via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 7 13:29:30 PDT 2024


https://github.com/kazutakahirata commented:

I think the on-disk header size is very useful.  People can keep adding new sections without affecting others who are not interested in the new sections.

I am not to sure about the minimum compatible version.  I have a couple of concerns:

- When somebody bumps the minimum compatible version due to an update to one specific section, they would teach `InstrProfWriter::minCompatibleReaderVersion` to return `Version14` or some higher number.  This is a profile-wide number, not the version of some specific section.  This means that users lose the forward compatibility regardless of whether they are using the updated section.
- The complexity is another concern.  You and I have discussed the min compatible version in person, so I understand it.  However, when somebody comes along and wishes to add or modify a section, it's a challenge to understand how it works.  Cursory reading of the source code most likely would not help them answer questions like "I'm modifying an existing section.  Should I add that as a new section?  Or should I modify the existing section and update the min compatible version?"

In any event, there should be some documentation, either as comments in the code or some `.rst`, about how to add and modify a section.  Relevant functions and variables should point to the documentation.

https://github.com/llvm/llvm-project/pull/88212


More information about the llvm-commits mailing list