Page MenuHomePhabricator

[ThinLTO] Bitcode reading/writing support for ThinLTO function summary/index

Authored by tejohnson on Aug 3 2015, 8:26 AM.



This patch adds bitcode reading/writing support for the ThinLTO function summary/index in bitcode. Some of the support is currently stubbed out (e.g. the records within the ThinLTO blocks), pending comments on the associated file format RFC.

The ThinLTO data structures are defined in

Related RFCs:
RFC: ThinLTO File Format (
RFC: ThinLTO File API and Data Structures (

Diff Detail

Event Timeline

tejohnson updated this revision to Diff 31234.Aug 3 2015, 8:26 AM
tejohnson retitled this revision from to [ThinLTO] Bitcode reading/writing support for ThinLTO function summary/index.
tejohnson updated this object.
tejohnson updated this object.Aug 3 2015, 8:31 AM
tejohnson updated this revision to Diff 31946.Aug 12 2015, 8:58 AM
tejohnson added reviewers: rafael, dexonsmith.

Updated the patch to use the parameter defined under -fthinlto in pending patch Added the same reviewers to this patch.

reames added a subscriber: reames.Aug 12 2015, 1:57 PM

Can you add references to the relevant RFCs to the review descriptions? It's getting hard to track all the pieces. My main comments can be found in the linked review for the data structures.


As a general comment: this index really isn't a ThinkLTO*Index, it's a *Index for the bitcode that happens to be used for LTO. The naming of the various methods and comments need to reflect that.

Can you add references to the relevant RFCs to the review descriptions? It's getting hard to track all the pieces. My main comments can be found in the linked review for the data structures.

Will do.


I responded to this suggestion in the review for D11721 as well - I didn't think this information would be used outside of ThinLTO importing decisions/mechanics. So thought it was better to name it accordingly. I'm open to changing the name, which currently also matches the names of the proposed bitcode sections.

tejohnson updated this object.Aug 12 2015, 2:36 PM
tejohnson updated this revision to Diff 32027.Aug 12 2015, 9:23 PM
tejohnson updated this object.

Removed native object wrapper support from this patch.

davidxl added inline comments.

Add a comment here to make it clear that lazy summary parsing is needed for reading combined index in the function import phase.


Make the comment clearer that:

  1. When reading combined index (ThinLTOSummaryIndex), getThinLTOIndex is first called with ParseFuncSummary=false
  2. The method supports lazily reading Function summary data associated for the function specified by the function name.

Should it really be 'readThinLTOFunctionSummary'?


parseBitcodeInto sounds too generic. Perhaps parseSummaryIndex?


Is it better to call it 'parseFunctionSummary'? Also move this interface decl after parseThinLTOSummary() below to show their order in the parsing hierarchy.


Do these parsing interfaces need to be public, other than the top level parseBitcodeInto and the lazy reading parseFunction?


Is it clearer to call it

parseThinLTOSummaries or parseThinLTOSummaryBlock?


parseBitCodeInto is a top level interface which is basically a wrapper to parseModule after seeking to the start of module block -- so it seems that parseModule can be inlined into parseBitcodeInto to avoid confusion.


Add comment about the purpose of the function.


Reorder this case to reflect the physical order of block ids on disk?


It is more readable to have a helper function to do 'seekToModuleStart' which includes the loop, and move the parseModule outside of it.


Suggested comment:

Otherwise skip the function summary section, but only create an index object with a map from function name to function summary offset. The index is used to perform lazy function summary reading later.



tejohnson marked 11 inline comments as done.Aug 18 2015, 5:38 PM

Responses and some other comments below. New patch being uploaded shortly.


Yes, changed.


Since it mirrors BitcodeReader::parseBitcodeInto, I will change this to parseSummaryIndexInto.


Changed the name. Can't move after parseThinLTOSummary since this one needs to be public and I am making the other one private (see below). Instead added a comment about it being used to read a function summary entry lazily.


Moved all of these parseThinLTO* routines into the private section.


Changed to parseEntireThinLTOSummary.


I prefer to keep the current division as it mirrors how parseBitcodeInto/parseModule are structured in the normal BilcodeReader.


I purposely structured this the same way as the corresponding BitcodeReader::parseBitcodeInto() function.


Inlined into getThinLTOIndex since that was the only caller.


Inlined into hasThinLTOIndex since that was the only caller.


Inlined into readThinLTOFunctionInfo since that was the only caller.


I have replaced this TODO with the code.

tejohnson updated this revision to Diff 32483.Aug 18 2015, 5:50 PM
tejohnson marked 5 inline comments as done.

Changes based on review David's review comments. I also added lots of other comments, and replaced a TODO in readThinLTOFunctionSummary to actually walk through the infos for a function name and access the saved summary offset.