[llvm] r255909 - [ThinLTO] Metadata linking for imported functions

Tue Jan 5 09:07:19 PST 2016

On Mon, Jan 4, 2016 at 1:19 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Dec-29, at 18:00, Teresa Johnson <tejohnson at google.com> wrote:
>>
>> On Wed, Dec 23, 2015 at 8:34 AM, Duncan P. N. Exon Smith
>> <dexonsmith at apple.com> wrote:
>>>
>>>> On 2015-Dec-17, at 12:14, Teresa Johnson via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>>>
>>>> Author: tejohnson
>>>> Date: Thu Dec 17 11:14:09 2015
>>>> New Revision: 255909
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=255909&view=rev
>>>> Log:
>>>> [ThinLTO] Metadata linking for imported functions
>>>>
>>>> Summary:
>>>> Second patch split out from http://reviews.llvm.org/D14752.
>>>>
>>>> Maps metadata as a post-pass from each module when importing complete,
>>>> suturing up final metadata to the temporary metadata left on the
>>>> imported instructions.
>>>>
>>>> This entails saving the mapping from bitcode value id to temporary
>>>> metadata in the importing pass, and from bitcode value id to final
>>>> metadata during the metadata linking postpass.
>>>>
>>>> Depends on D14825.
>>>>
>>>> Reviewers: dexonsmith, joker.eph
>>>>
>>>> Subscribers: davidxl, llvm-commits, joker.eph
>>>>
>>>> Differential Revision: http://reviews.llvm.org/D14838
>>>>
>>>> Added:
>>>>   llvm/trunk/test/Linker/Inputs/thinlto_funcimport_debug.ll
>>>>   llvm/trunk/test/Linker/thinlto_funcimport_debug.ll
>>>>   llvm/trunk/test/Transforms/FunctionImport/Inputs/funcimport_debug.ll
>>>>   llvm/trunk/test/Transforms/FunctionImport/funcimport_debug.ll
>>>> Modified:
>>>>   llvm/trunk/include/llvm/IR/GVMaterializer.h
>>>>   llvm/trunk/include/llvm/IR/Metadata.h
>>>>   llvm/trunk/include/llvm/IRReader/IRReader.h
>>>>   llvm/trunk/include/llvm/Linker/IRMover.h
>>>>   llvm/trunk/include/llvm/Linker/Linker.h
>>>>   llvm/trunk/include/llvm/Transforms/Utils/ValueMapper.h
>>>>   llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
>>>>   llvm/trunk/lib/IR/Metadata.cpp
>>>>   llvm/trunk/lib/IRReader/IRReader.cpp
>>>>   llvm/trunk/lib/Linker/IRMover.cpp
>>>>   llvm/trunk/lib/Linker/LinkModules.cpp
>>>>   llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
>>>>   llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp
>>>>   llvm/trunk/tools/llvm-link/llvm-link.cpp
>>>>
>>>>
>>>> Modified: llvm/trunk/include/llvm/IR/Metadata.h
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Metadata.h?rev=255909&r1=255908&r2=255909&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/include/llvm/IR/Metadata.h (original)
>>>> +++ llvm/trunk/include/llvm/IR/Metadata.h Thu Dec 17 11:14:09 2015
>>>> @@ -832,10 +832,11 @@ public:
>>>>  /// \brief Resolve cycles.
>>>>  ///
>>>>  /// Once all forward declarations have been resolved, force cycles to be
>>>> -  /// resolved.
>>>> +  /// resolved. If \p MDMaterialized is true, then any temporary metadata
>>>> +  /// is ignored, otherwise it asserts when encountering temporary metadata.
>>>
>>> Please expand a little on the semantic difference between `true` and
>>> `false` here.  In the `false` case, you're blocking identical `MDNode`s
>>> from getting uniqued together.
>>
>> Looks like I got the comment backwards. We ignore temp metadata when
>> MDMaterialized==false, otherwise we assert when seeing temp metadata
>> here (the default behavior). I don't think there is any other semantic
>> difference - the flag only affects temporary MD, which we shouldn't
>> normally see (which is why the default is the old behavior of
>> asserting on temp metadata).
>
> It's subtle, but I think it's important.  Consider the following.

Thanks for walking me through this, I see the issue. Comments below.

> --
> The LLVMContext has:
>     !1 = !{!2}
>     !2 = !{}
>
> a.bc contains:
>     !1 = !{!2}
>     !2 = !{}
> --
> The full-loader would read this in, and merge !1 and !2 together.
> --
> Let's say the lazy-loader chooses *only* to read !1 from a.bc,
> leaving !2 as a temporary.  Now LLVMContext has:
>     !1 = !{!2}
>     !2 = !{}
>     !3 = unresolved !{!4} // !1 from a.bc (with RAUW support)
>     !4 = temporary !{}    // !2 from a.bc
>
> Lazy-loader calls !3->resolveCycles(), giving:
>     !1 = !{!2}
>     !2 = !{}
>     !3 = !{!4}         // !1 from a.bc (no RAUW support)
>     !4 = temporary !{} // !2 from a.bc

Note that the lazy metadata reading support delays reading of all
module-level metadata, which it later reads in a single pass. So the
above situation would only happen if !1 from a.bc is in a
function-level metadata block, and !2 is in its module-level metadata.

>
> Eventually, we read in !4.  !3 hits a uniquing collision, but
> doesn't have RAUW support (since `resolveCycles()` dropped it).
> End result:
>     !1 = !{!2}
>     !2 = !{}
>     !3 = distinct !{!2} // !1 from a.bc
> --
> If `resolveCycles()` hadn't been called, we'd end up with:
>     !1 = !{!2}
>     !2 = !{}
> since !3 could resolve the uniquing collision using RAUW.

I wonder if it would be possible to delay calling resolveCycles until
the module-level metadata is loaded? That might result in other
issues/differences though?

> --
>
> The new operation completely violates the spirit of
> `resolveCycles()`, which (before your patch) was saying:
>     "I promise there are no more temporaries, so remaining
>     unresolved nodes must be part of a cycle."
> The `assert()` confirms that everything is in a cycle.
>
> With your new flag set, this has changed to:
>     "I don't care about uniquing the remaining nodes, so drop
>      RAUW support for them."
>
> Maybe something like this would make sense:
> --
> void resolveCycles()
> { resolveRecursivelyImpl(/* AllowTemps */ false); }
> void resolveNonTemporaries()
> { resolveRecursivelyImpl(/* AllowTemps */ true); }
>
> private:
> // Old resolveCycles() implementation.
> void resolveRecursivelyImpl(bool AllowTemps);
> --
> (Do you have a better name for `resolveNonTemporaries()`?)

This sounds like a good idea. Don't have a better name suggestion,
will use that.

>
> With the API split it might be easier to document the semantic
> differences (just by describing the use case for each).
>
> For `resolveCycles()`, please make clear (if it isn't already?)
> that we're promising there aren't any temporaries, and thus
> unresolved nodes are part of cycles and no longer need RAUW
> support.
>
> For `resolveNonTemporaries()`, please make clear that this is a
> destructive operation that blocks uniquing of metadata nodes.
>

Will do.

Thanks,
Teresa

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