This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Add string saver onto index for value names
ClosedPublic

Authored by tejohnson on Jun 6 2018, 12:46 PM.

Details

Summary

Adds a string saver to the ModuleSummaryIndex so it can store value
names in the case of adding a ValueInfo for a GUID when we don't
have the name stored in a Module string table. This is motivated
by the upcoming summary parser patch, where we will read value names
from the summary entry and want to store them, even when a Module
is not available.

Currently this allows us to store the name in the legacy bitcode case,
and I have added a test to show that.

Diff Detail

Event Timeline

tejohnson created this revision.Jun 6 2018, 12:46 PM

I forgot to add, StringSaver is not copyable (due to BumpPtrAllocator reference), therefore this required changes to the Index value on the ModuleSummaryIndexWrapperPass to hold a unique_ptr instead of Optional, so that it could be initialized without a copy.

evgeny777 added inline comments.
lib/Bitcode/Reader/BitcodeReader.cpp
4846

Probably reflect the purpose of this in comment above? Otherwise it's misleading.

tejohnson marked an inline comment as done.Jun 20 2018, 1:07 PM
tejohnson added inline comments.
lib/Bitcode/Reader/BitcodeReader.cpp
4846

Updated comment to reflect the new behavior.

tejohnson updated this revision to Diff 152139.Jun 20 2018, 1:07 PM
tejohnson marked an inline comment as done.

Address comment.

My patch update accidentally pulled in a separate patch, let me fix that now...

tejohnson updated this revision to Diff 152151.Jun 20 2018, 1:45 PM

Fixed patch and also rebased to HEAD

tejohnson updated this revision to Diff 152624.Jun 24 2018, 8:59 PM

As discussed on D47905, make the allocator and saver Optional. Also revert
change to make the ModuleSummaryIndexWrapperPass a unique_ptr instead of
Optional, as the emplace() method can be used to avoid the issues my change
introduced there.

This revision is now accepted and ready to land.Jun 25 2018, 1:54 PM
pcc added a comment.Jun 25 2018, 1:59 PM

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

Are you sure that is the case for Alloc? The BumpPtrAllocatorImpl has a number of data members.

pcc added a comment.Jun 25 2018, 3:39 PM
In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

Are you sure that is the case for Alloc? The BumpPtrAllocatorImpl has a number of data members.

Yes, the purpose of the AlignedCharArrayUnion in OptionalStorage is to be at least as large and aligned as T. And Optional contains a field of type OptionalStorage.

In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

My recollection is that BumpPtrAllocator does its first heap allocation on construction. The point was to avoid that allocation when it was unnecessary. Have I remembered incorrectly?

pcc added a comment.Jun 25 2018, 3:50 PM
In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

My recollection is that BumpPtrAllocator does its first heap allocation on construction. The point was to avoid that allocation when it was unnecessary. Have I remembered incorrectly?

It doesn't appear to do that. BumpPtrAllocator's default constructor (http://llvm-cs.pcc.me.uk/include/llvm/Support/Allocator.h#152) uses = default;.

In D47842#1142966, @pcc wrote:
In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

My recollection is that BumpPtrAllocator does its first heap allocation on construction. The point was to avoid that allocation when it was unnecessary. Have I remembered incorrectly?

It doesn't appear to do that. BumpPtrAllocator's default constructor (http://llvm-cs.pcc.me.uk/include/llvm/Support/Allocator.h#152) uses = default;.

Okay. Then I don't see a benefit of Optional vs just using them directly.

In D47842#1142953, @pcc wrote:
In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

Are you sure that is the case for Alloc? The BumpPtrAllocatorImpl has a number of data members.

Yes, the purpose of the AlignedCharArrayUnion in OptionalStorage is to be at least as large and aligned as T. And Optional contains a field of type OptionalStorage.

Ok I see. Per the follow on discussion, I will revert this part of the change before committing.

tejohnson updated this revision to Diff 152823.Jun 25 2018, 7:33 PM

Revert change to make Alloc/Saver Optional.

This revision was automatically updated to reflect the committed changes.