[llvm] r255909 - [ThinLTO] Metadata linking for imported functions
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
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
More information about the llvm-commits
mailing list