[PATCH] D35189: Add GUID-ValueID map and original name to ThinLTO summary.

[Dehao Chen via llvm-commits]

On Mon, Jul 10, 2017 at 11:20 AM, Teresa Johnson via Phabricator
<reviews at reviews.llvm.org> wrote:
> tejohnson added a comment.
>
> In https://reviews.llvm.org/D35189#803977, @haojiewang wrote:
>
>> In https://reviews.llvm.org/D35189#803953, @mehdi_amini wrote:
>>
>> > In https://reviews.llvm.org/D35189#803910, @haojiewang wrote:
>> >
>> > > In https://reviews.llvm.org/D35189#803332, @mehdi_amini wrote:
>> > >
>> > > > The context and the motivation is *still* not clear to me. To begin why do we need the *names* when doing the thin link?
>> > >
>> > >
>> > > We need the GUID because it is needed when we build the combined index in thin link phase. We need ValueId is because the size of GUID is too large so that we do not want to write it everywhere, so we set a ValueId which is indexing from 0. And we should also set a GUID to ValueId map to record the mapping between them.
>> > >  We need the original name because it is needed in FunctionImport. So we should pass is to thin link phase.
>> >
>> >
>> > You manage to not really answer fully my question here. But I think infer it from the code in my previous inline comment, it is "we need the name to recompute a GUID for internal symbol promoted to global", correct?
>>
>>
>> Just talked to Teresa, the FunctionImport seems no longer need the original name(but seems that there are still some code in FunctionImport that refer original name), but PGO will use this information.
>
>
> To clarify, I believe Sample PGO is using the original name (as opposed to instrumentation PGO). Dehao?

Yes, the original name is needed by samplePGO for proper ICP of static
functions. Originally added in https://reviews.llvm.org/D30754

Dehao

>
> I am having trouble figuring out how we originally used this information, it was added here https://reviews.llvm.org/D19454 (I believe for the old workaround of not importing from modules with non-renamable values as discussed in https://reviews.llvm.org/D18298, which we now have a better fix for).
>
>
> https://reviews.llvm.org/D35189
>
>
>