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

[Duncan P. N. Exon Smith via llvm-commits]

> 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
--

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}