[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