Page MenuHomePhabricator

Support for function summary index bitcode sections and files
ClosedPublic

Authored by tejohnson on Sep 23 2015, 10:17 AM.

Details

Summary

The bitcode format is described in this document:

https://drive.google.com/file/d/0B036uwnWM6RWdnBLakxmeDdOeXc/view

For more info on ThinLTO see:

https://sites.google.com/site/llvmthinlto

The first customer is ThinLTO, however the data structures are designed
and named more generally based on prior feedback. There are a few
comments regarding how certain interfaces are used by ThinLTO, and the
options added here to gold (and to a separate clang patch which will
be mailed along with this) currently have ThinLTO-specific names as the
behavior they provoke is currently ThinLTO-specific.

This patch includes support for generating per-module function indexes,
the combined index file via the gold plugin, and several tests
(more are included with the associated clang patch).

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson updated this revision to Diff 35523.Sep 23 2015, 10:17 AM
tejohnson retitled this revision from to Support for function summary index bitcode sections and files. Includes bitcode reader/writer support for these sections, and support for creation of combined index files..
tejohnson updated this object.
tejohnson added a subscriber: llvm-commits.

Rather than create a new clang patch, I have updated http://reviews.llvm.org/D11908.

tejohnson updated this revision to Diff 35761.Sep 25 2015, 1:16 PM
  • Fix function summary generation for anonymous function.

Don't try to generate a summary for them as they don't have a
VST entry/value Id, and ensure that any aliases get function summaries instead.

This was caught by running llvm test suite with thinlto enabled by
default in llvm-as.

Add anonymous function to thinlto test case.

mehdi_amini edited edge metadata.Sep 30 2015, 10:33 PM

Hi Teresa,

Please find some inline comments below. I had some minor naming suggestion, but more important is the ownership of some object instantiation.

include/llvm/Bitcode/BitcodeWriterPass.h
54 ↗(On Diff #35761)

Rename ThinLTO with FunctionSummary to be coherent with the rest like hasFunctionSummary() for instance?

I think there are multiple places (not to say everywhere) where ThinLTO could be replaced with something more generic like FunctionSummary.

include/llvm/Bitcode/BitstreamWriter.h
24 ↗(On Diff #35761)

Why?

528 ↗(On Diff #35761)

Shouldn't be needed?

include/llvm/Bitcode/ReaderWriter.h
74 ↗(On Diff #35761)

Might be more explicit to rename ParseFuncSummary into IsLazy.
(just a suggestion, do whatever seems better to you)

include/llvm/IR/FunctionInfo.h
45 ↗(On Diff #35761)

s/disambiguiate/disambiguate/ ?

99 ↗(On Diff #35761)

The "it" at the beginning of the line seems strange to me?

107 ↗(On Diff #35761)

Who owns it?
(I may found out later)

108 ↗(On Diff #35761)

(Missing empty line)

130 ↗(On Diff #35761)

If this takes ownership, use a unique_ptr for the argument.

136 ↗(On Diff #35761)

Any reason not to use unique_ptr for the Summary member?

143 ↗(On Diff #35761)

This sounds suspicious: a copy ctor that does not really copy...
I have to see the use case first, I'm skeptical right now :)

166 ↗(On Diff #35761)

The other setters name are beginning with set, why not this one?

190 ↗(On Diff #35761)

remove

213 ↗(On Diff #35761)

Why not using unique_ptr and remote the destructor?

223 ↗(On Diff #35761)

If it was called "begin()" you could use for-range loop. An alternative would be to add another method functions() that would return an llvm::iterator_range.

232 ↗(On Diff #35761)

Do you really intend to return a copy of the vector all the time?

include/llvm/Object/FunctionIndexObjectFile.h
84 ↗(On Diff #35761)

indentation

lib/Bitcode/Reader/BitcodeReader.cpp
410 ↗(On Diff #35761)

What about a variable name that contains Lazy?

437 ↗(On Diff #35761)

The ownership/lifetime of FunctionSummary is not clear here?

3110 ↗(On Diff #35761)

remove

5215 ↗(On Diff #35761)

Should you just return now?

5241 ↗(On Diff #35761)

I would expect clang-format to put the else on the same line as the {?

5313 ↗(On Diff #35761)

I don't see the expected return for this function?

5343 ↗(On Diff #35761)

The indentation seems incorrect, is it clang-formatted?

5382 ↗(On Diff #35761)

Typo with ThihLTO.
(also I'm still not sure at which level where the "ThinLTO" concept should "leak").

5684 ↗(On Diff #35761)

When is this memory released?

lib/Bitcode/Writer/BitcodeWriter.cpp
2304 ↗(On Diff #35761)

For-range loops?

2323 ↗(On Diff #35761)

for(auto P : FuncName) ?

2384 ↗(On Diff #35761)

Ownership is not clear, what about having a DenseMap<const Function *, std::unique_ptr<FunctionInfo>> instead?

2693 ↗(On Diff #35761)

for (auto P : MPSE.getKey()) ?

2720 ↗(On Diff #35761)

For-range loop?

2757 ↗(On Diff #35761)

Some refactoring seems possible between these two loops.

2780 ↗(On Diff #35761)

for-range loop?

2994 ↗(On Diff #35761)

(Side note) I read this so many times, I'm surprised there is not a better refactoring for reading/writing bitcode.

lib/IR/FunctionInfo.cpp
19 ↗(On Diff #35761)

A mutable global is really to be avoided. I am really not convinced that it is desirable here.
Should it be passed by argument to the mergeFrom method? Or a member variable?

Maybe even mergeFrom() should not be a member function, but a separate object that would have a NextModuleId and you could add FunctionInfoIndex and get the merged result.

23 ↗(On Diff #35761)

What about taking a unique_ptr here and "stealing" its content instead of doing a bunch of redundant memory allocation and copy?

29 ↗(On Diff #35761)

This loop header is strange with the iterator declared outside. Turn into a for-range loop?

lib/Object/FunctionIndexObjectFile.cpp
89 ↗(On Diff #35761)

I'm not sure why you would need to provide the fully qualified name for this function?

test/Bitcode/thinlto-function-summary.ll
17 ↗(On Diff #35761)

This line is ignored

25 ↗(On Diff #35761)

This line is ignored as well

test/tools/gold/X86/thinlto.ll
8 ↗(On Diff #35761)

What is this line doing? Isn't the -a for the && operator?
Do you check that the file %t3 is not generated by gold and only the combine index is?

tejohnson updated this revision to Diff 36432.Oct 3 2015, 8:22 AM
tejohnson edited edge metadata.
  • Address Mehdi's review comments.
mehdi_amini added inline comments.Oct 3 2015, 11:58 AM
lib/Bitcode/Reader/BitcodeReader.cpp
5659 ↗(On Diff #36432)

Why not having R as a stack local variable? Initially before the unique_ptr I thought you were intended to pass it outside but it seems to be a pure local variable.

test/Bitcode/thinlto-function-summary.ll
18 ↗(On Diff #36432)

I didn't see the second RUN: line, forget this.

tejohnson updated this revision to Diff 36449.Oct 3 2015, 3:30 PM
  • Make FunctionIndexBitcodeReader a local variable.
tejohnson retitled this revision from Support for function summary index bitcode sections and files. Includes bitcode reader/writer support for these sections, and support for creation of combined index files. to Support for function summary index bitcode sections and files.Oct 4 2015, 7:21 AM
This revision was automatically updated to reflect the committed changes.

Hi Teresa,

A couple of inline comments, I'll have more, but I just thought I'd send a couple out.

One general comment:

How expensive is the index (size and time to compute)? Is it worth always including in the module?

Thanks!

-eric

llvm/trunk/include/llvm/IR/FunctionInfo.h
57

Comment nit: "during the initial compile step" seems weird and not wholly accurate.

Relatedly: would a transform be expected to keep it up to date? Is it recomputed on every write? (Seems reasonable, but costly).

61–62

This comment seems a bit confusing. Could you elaborate more?