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

[Teresa Johnson via llvm-commits]

On Mon, Jul 10, 2017 at 11:04 AM, Mehdi AMINI <joker.eph at gmail.com> wrote:

>
>
> 2017-07-10 9:58 GMT-07:00 Teresa Johnson <tejohnson at google.com>:
>
>>
>>
>> On Mon, Jul 10, 2017 at 9:16 AM, Mehdi AMINI via Phabricator <
>> reviews at reviews.llvm.org> wrote:
>>
>>> mehdi_amini added a comment.
>>>
>>>
>>>
>>>
>>>
>>> ================
>>> Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3437
>>> +  // For local linkage, we also emit the original name separately
>>> +  // immediately after the record.
>>> +  auto MaybeEmitOriginalName = [&](const GlobalValue &GV) {
>>> ----------------
>>> Please add here *why* this is done, other the comment is fairly not
>>> helpful: it just tells me wha the function is doing.
>>> I think the reason is "This is needed because promoting internal symbol
>>> to global requires generating a new GUID from the original name".
>>> That said, unless there is another reason I missed, we could also
>>> immediately instead store two GUID for internal symbols: the local and the
>>> global.
>>>
>>
>> I suggested Haojie use the same mechanism we use in the combined index
>> for storing the original GUID (which is why the FS_COMBINED_ORIGINAL_NAME
>> was renamed/repurposed to FS_ORIGINAL_NAME in this patch). Otherwise we
>> should rework the combined index for consistency, and I'm not sure that is
>> worthwhile.
>>
>> Also, alternatively and even more simpler I think, we could just always
>>> use the global GUID for internal symbols, whether they're promoted or not.
>>>
>>
>> This will cause issues with PGO indirect call promotion + ThinLTO.
>>
>
> What kind? I thought we had a mechanism for this with some metadata
> recording the PGO guid or something like that.
>

The indirect call metadata records the GUID of the callee. We need to match
that GUID to one in the index so we can import the right function. The PGO
GUID uses the same naming scheme of "modulename:valuename".

>
>
>
>> And would result in name clashes in the combined index (the internal
>> symbols get a GUID computed from the hash of "modulename:valuename", see
>> GlobalValue::getGlobalIdentifier).
>>
>
> This isn't clear to me: I was suggesting using *only* the GUID that can't
> clash (i.e. the one computed from "modulename:valuename").
>

Then the GUID will not match the PGO data for global indirect callees.


>
> --
> Mehdi
>
>
>
>


-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170710/75f85633/attachment-0001.html>