[PATCH] D14447: Restore "Move metadata linking after lazy global materialization/linking."

[Teresa Johnson via llvm-commits]

On Fri, Nov 6, 2015 at 9:43 AM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Nov-06, at 08:21, Teresa Johnson <tejohnson at google.com> wrote:
>>
>> tejohnson created this revision.
>> tejohnson added reviewers: dexonsmith, rafael, tra.
>> tejohnson added subscribers: llvm-commits, joker.eph, tobiasvk.
>>
>> This reverts commit r251965.
>>
>> Restore "Move metadata linking after lazy global materialization/linking."
>>
>> This restores commit r251926, with fixes for the LTO bootstrapping bot
>> failure.
>>
>> The bot failure was caused by references from debug metadata to
>> otherwise unreferenced globals. Previously, this caused the lazy linking
>> to link in their defs, which is unnecessary. With this patch, because
>> lazy linking is complete when we encounter the metadata reference, the
>> materializer created a declaration. For definitions such as aliases and
>> comdats, it is illegal to have a declaration. Furthermore, metadata
>> linking should not change code generation. Therefore, when linking of
>> global value bodies is complete, the materializer will simply return
>> nullptr as the new reference for the linked metadata.
>>
>> This change required fixing a different test to ensure there was a
>> real reference to a linkonce global that was only being reference from
>> metadata.
>>
>> Note that the new changes to the only-needed-named-metadata.ll test
>> illustrate an issue with llvm-link -only-needed handling of comdat
>> groups, whereby it may result in an incomplete comdat group. I note this
>> in the test comments, but the issue is orthogonal to this patch (it can
>> be reproduced without any metadata at head).
>>
>> http://reviews.llvm.org/D14447
>>
>> Files:
>>  lib/Linker/LinkModules.cpp
>>  test/Linker/Inputs/only-needed-named-metadata.ll
>>  test/Linker/distinct.ll
>>  test/Linker/only-needed-named-metadata.ll
>>
>> <D14447.39540.patch>
>
> LGTM with a minor testcase nitpick:
>
>> Index: test/Linker/distinct.ll
>> ===================================================================
>> --- test/Linker/distinct.ll
>> +++ test/Linker/distinct.ll
>> @@ -6,6 +6,12 @@
>>
>>  ; CHECK: @global = linkonce global i32 0
>>  @global = linkonce global i32 0
>> +; Add a reference to @global since linkonce are lazy linked only upon real
>> +; references from code.
>> +define void @func() {
>> +  load i32, i32* @global
>> +  ret void
>> +}
>
> I think an alias would be a simpler reference, to keep the testcase
> simple:
> --
> ; Add an external reference to @global so that it gets linked in.
> @alias = alias i32* @global

Thanks, went with the alias approach.

> --
>
> Another option (although it takes more characters) is to add @global to
> the @llvm.used global array.
>
>>
>>  ; CHECK: !named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !0, !1, !2, !9, !10, !11, !12, !13, !14}
>>  !named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8}
>



-- 
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413