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

Mon Jan 4 13:22:00 PST 2016

> On 2015-Dec-30, at 09:50, Teresa Johnson <tejohnson at google.com> wrote:
> 
> On Tue, Dec 29, 2015 at 6:00 PM, 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/lib/Bitcode/Reader/BitcodeReader.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=255909&r1=255908&r2=255909&view=diff
>>>> ==============================================================================
>>>> @@ -3062,6 +3081,30 @@ std::error_code BitcodeReader::materiali
>>>> 
>>>> void BitcodeReader::setStripDebugInfo() { StripDebugInfo = true; }
>>>> 
>>>> +void BitcodeReader::saveMDValueList(
>>>> +    DenseMap<const Metadata *, unsigned> &MDValueToValIDMap, bool OnlyTempMD) {
>>> 
>>> "MDValue" is a horrible term that I wish I'd never come up with.
>>> Please don't propagate it.  If you get a chance, please clean up
>>> instances of it (even the ones you didn't add).
>>> 
>>> I think this should be called `saveMetadataList()`, and the map
>>> should be `MetadataToIDs`.
>> 
>> I did both of these just now in r256593.
>> 
>>> 
>>>> +  for (unsigned ValID = 0; ValID < MDValueList.size(); ++ValID) {
>>> 
>>> I suggest just using `ID` here instead of `ValID`.
>> 
>> Ditto
>> 
>>> 
>>>> +    Metadata *MD = MDValueList[ValID];
>>>> +    auto *N = dyn_cast_or_null<MDNode>(MD);
>>>> +    // Save all values if !OnlyTempMD, otherwise just the temporary metadata.
>>>> +    if (!OnlyTempMD || (N && N->isTemporary())) {
>>> 
>>> Why do you need to record metadata that isn't an MDNode here (i.e., why
>>> not `continue` if `!N`)?
>> 
>> I don't remember, but will take a look if I can change that. (I just
>> did and am getting an assert in one of the ThinLTO regression tests,
>> need to see if I just need to modify a use or something).
> 
> Can't do this as there are ValueAsMetadata in the module level
> metadata bitcode that are referenced from instructions (the example I
> found was from an llvm.dbg.value instruction).Therefore, the
> instruction gets a temp MD node during function importing, and we need
> to record the ValueAsMetadata here as well so that the temp MD is
> replaced with it during metadata linking. Will add a comment.

Okay, makes sense.

>> 
>>> 
>>> Also, it would be great to be able to assert:
>>> --
>>> assert(N->isResolved() || N->isTemporary());
>>> --
>>> This confirms that you've called `resolveCycles()`.  If you haven't
>>> called `resolveCycles()` by now, then I don't think it's safe to put `N`
>>> in the map.
>> 
>> Ok, will try that.
> 
> Added the assert, although modified to handle the !N case, and that
> works fine as expected.

Great.

>> 
>>> 
>>> Finally, there's an important missing assertion that I'm not sure how
>>> to provide.  But it's important.
>>> 
>>> IIUC, it's vital that this *doesn't* happen:
>>> 
>>> 1. Temporary metadata T is put into MetadataToIDs.
>>> 2. T.replaceAllUsesWith(MD) gets called.
>>> 3. MetadataToIDs.count/find/etc.(T) or (MD).
>>> 
>>> First case (T): since `T` has been freed, some new temporary (or normal)
>>> metadata might get allocated in the same spot.  Someone could be
>>> checking for this new `N`, and get the ID for the old `T`.
>>> 
>>> Second case (MD): the map contains the ID for `MD`, but it's indexed
>>> under the old pointer `T`.  Checking for `MD` will give the wrong answer.
>>> 
>>> As I mentioned, I don't know how to add an assertion or verification for
>>> this, but I think it's really important.  Please look into how to
>>> verify that these cases can't occur.
>> 
>> In the case where we are saving temporary metadata in the
>> MetadataToIDs map, we should never replace it once we save it to that
>> map, until after we delete that instance of the MetadataToIDs map when
>> the IRLinker is destructed.
>> 
>> So what I could do is when we save temp metadata to the map, set a
>> flag on the temp MD's ReplaceableMetadataImpl, and assert in
>> ReplaceableMetadataImpl::replaceAllUsesWith if the flag is set, then
>> clear the flag when we destruct the IRLinker and its associated
>> MetadataToIDs map. Does that seem reasonable? I'll give it a shot.
> 
> This works. I also confirmed that I get an assert during metadata
> linking if I skip resetting the flag during IRLinker destruction, so
> it does catch a RAUW on temporary data with the flag set.
> 
> Will commit these new asserts and fix the other issues you pointed out
> during your review.

Awesome, thanks.  You might be able to add a unit test using
`EXPECT_DEATH()`.