This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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)

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

include/llvm/Bitcode/ReaderWriter.h
70

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.

include/llvm/Bitcode/ReaderWriter.h
70

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.
include/llvm/Bitcode/ReaderWriter.h
73

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

77

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.
78

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.

4737

Add comment about the purpose of the function.

4803

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

4988

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

5276

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.

5331

FunctionSummaryOffset?

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

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

include/llvm/Bitcode/ReaderWriter.h
78

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.

4988

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

5251

Inlined into getThinLTOIndex since that was the only caller.

5286

Inlined into hasThinLTOIndex since that was the only caller.

5312

Inlined into readThinLTOFunctionInfo since that was the only caller.

5325

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.