This is an archive of the discontinued LLVM Phabricator instance.

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

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
60

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

Why?

527

Shouldn't be needed?

include/llvm/Bitcode/ReaderWriter.h
74

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

include/llvm/IR/FunctionInfo.h
46

s/disambiguiate/disambiguate/ ?

100

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

108

Who owns it?
(I may found out later)

109

(Missing empty line)

131

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

137

Any reason not to use unique_ptr for the Summary member?

144

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

167

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

191

remove

214

Why not using unique_ptr and remote the destructor?

224

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.

233

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

include/llvm/Object/FunctionIndexObjectFile.h
85

indentation

lib/Bitcode/Reader/BitcodeReader.cpp
410

What about a variable name that contains Lazy?

437

The ownership/lifetime of FunctionSummary is not clear here?

3108

remove

5213

Should you just return now?

5239

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

5311

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

5341

The indentation seems incorrect, is it clang-formatted?

5380

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

5659

When is this memory released?

lib/Bitcode/Writer/BitcodeWriter.cpp
2304

For-range loops?

2323

for(auto P : FuncName) ?

2376

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

2683

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

2710

For-range loop?

2747

Some refactoring seems possible between these two loops.

2770

for-range loop?

2980

(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
20

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.

24

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

30

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

lib/Object/FunctionIndexObjectFile.cpp
90

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

test/Bitcode/thinlto-function-summary.ll
18

This line is ignored

26

This line is ignored as well

test/tools/gold/X86/thinlto.ll
9

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

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
19

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 ↗(On Diff #36467)

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 ↗(On Diff #36467)

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