[PATCH] D18763: [ThinLTO] Move summary computation from BitcodeWriter to new pass

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In http://reviews.llvm.org/D18763#392671, @joker.eph wrote:

> I think this introduces a distinction between the module level summary and the combined index, both in the memory representation and the usage (the client would deal differently with both).
>  I wonder if it is not a sign that some more restructuring could make it cleaner, with a separate class for the combined index and the module summary one.


There's so much overlap between them that I don't know that separating them would be helpful.

Note that in the current upstream implementation I am actually writing value ids into the summary entries populated during bitcode writing in the per module case, they are essentially standing in for GUIDs in the per-module case, and no type change was required since they are unsigned ints and therefore the same uint64_t field works for either these value ids or the GUIDs in the combined index case.

Here I had to make the different representation in the per-module vs combined cases explicit since we don't yet have the value ids from the ValueEnumerator available.

> Also I'm not totally sure why we're still using value id in the module summary case and GUID for the combined index?


Note that they *both* use a value id in the bitcode representation. In both cases the value ids are assigned by the bitcode writer:

- In the per-module case this value id is assigned by the ValueEnumerator which is essentially private to the bitcode writer, and the value ids used for writing the summary must use those (since the summary leverages the same VST used by the rest of the bitcode).
- In the combined case the value ids used in the combined VST are assigned to each GUID via a map populated as we write the combined summary entries. As mentioned on IRC yesterday, this is to be more compact that using a much larger GUID in all of the reference graph entries.

It doesn't make sense to force creation of a GUID in the per-module case since we somehow need to get back to the Value* to access the real value ID used in the VST from the ValueEnumerator.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:76
@@ +75,3 @@
+  /// Accessor for GUID value
+  GlobalValue::GUID getId() const {
+    assert(Kind == VI_Id && "Not an Id type");
----------------
joker.eph wrote:
> `getGUID` might be clearer at call sites?
Good idea


http://reviews.llvm.org/D18763