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

[Mehdi AMINI via llvm-commits]

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.



> 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").

-- 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170710/e119c07f/attachment.html>