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

[tejohnson]

Summary

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 http://reviews.llvm.org/D11721.

Related RFCs:
RFC: ThinLTO File Format (http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-August/088947.html)
RFC: ThinLTO File API and Data Structures (http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-August/088951.html)

[tejohnson]

Updated the patch to use the parameter defined under -fthinlto in pending patch http://reviews.llvm.org/D11907. Added the same reviewers to this patch.

[reames]

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.

include/llvm/Bitcode/ReaderWriter.h

69

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.

[tejohnson]

In D11722#223090, @reames wrote:

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.

include/llvm/Bitcode/ReaderWriter.h

69

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]

Removed native object wrapper support from this patch.

[davidxl]

include/llvm/Bitcode/ReaderWriter.h

72

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

76

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.

77

Should it really be 'readThinLTOFunctionSummary'?

lib/Bitcode/Reader/BitcodeReader.cpp

425

parseBitcodeInto sounds too generic. Perhaps parseSummaryIndex?

427

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

431

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

433

Is it clearer to call it

parseThinLTOSummaries or parseThinLTOSummaryBlock?

437

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.

4865

Add comment about the purpose of the function.

4931

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

5116

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

5409

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.

5464

FunctionSummaryOffset?

[tejohnson]

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

include/llvm/Bitcode/ReaderWriter.h

77

Yes, changed.

lib/Bitcode/Reader/BitcodeReader.cpp

425

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

427

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.

431

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

433

Changed to parseEntireThinLTOSummary.

437

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

5116

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

5384

Inlined into getThinLTOIndex since that was the only caller.

5419

Inlined into hasThinLTOIndex since that was the only caller.

5445

Inlined into readThinLTOFunctionInfo since that was the only caller.

5458

I have replaced this TODO with the code.

[tejohnson]

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.

[tejohnson]

Obsolete. New patch is http://reviews.llvm.org/D13107