[PATCH] D14838: [ThinLTO] Metadata linking for imported functions

[Teresa Johnson via llvm-commits]

On Mon, Dec 7, 2015 at 11:31 AM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Dec-07, at 06:54, Teresa Johnson <tejohnson at google.com> wrote:
>>
>> On Sat, Dec 5, 2015 at 6:10 PM, Duncan P. N. Exon Smith
>> <dexonsmith at apple.com> wrote:
>>>
>>>> On 2015-Dec-04, at 11:12, Teresa Johnson <tejohnson at google.com> wrote:
>>>>
>>>> On Wed, Dec 2, 2015 at 2:04 PM, Teresa Johnson <tejohnson at google.com> wrote:
>>>>> On Tue, Dec 1, 2015 at 5:11 PM, Duncan P. N. Exon Smith
>>>>> <dexonsmith at apple.com> wrote:
>>>>>>
>>>>>>> On 2015-Nov-30, at 07:05, Teresa Johnson <tejohnson at google.com> wrote:
>>>>>>>
>>>>>>> tejohnson added a comment.
>>>>>>>
>>>>>>> Ping.
>>>>>>> Thanks, Teresa
>>>>>>>
>>>>>>>
>>>>>>> http://reviews.llvm.org/D14838
>>>>>>
>>>>>> Thanks for working on this, and sorry for the delay.
>>>>>>
>>>>>> I was looking at the patch bottom-up and came across a major issue, so
>>>>>> I've stopped reviewing at `TrackingMDRef`.  Once you've got that solved
>>>>>> I can look again at the updated patch.  However, it's quite big, so if
>>>>>> you can possibly split out the uninteresting/separable bits that would
>>>>>> be helpful.
>>>>>
>>>>> Thanks for the comments. Response below. Will look for places to split
>>>>> out some of the simpler stuff that can go in separately.
>>>>>
>>>>>>
>>>>>>> Index: include/llvm/IR/TrackingMDRef.h
>>>>>>> ===================================================================
>>>>>>> --- include/llvm/IR/TrackingMDRef.h
>>>>>>> +++ include/llvm/IR/TrackingMDRef.h
>>>>>>> @@ -14,6 +14,7 @@
>>>>>>> #ifndef LLVM_IR_TRACKINGMDREF_H
>>>>>>> #define LLVM_IR_TRACKINGMDREF_H
>>>>>>>
>>>>>>> +#include "llvm/ADT/DenseMapInfo.h"
>>>>>>> #include "llvm/IR/MetadataTracking.h"
>>>>>>> #include "llvm/Support/Casting.h"
>>>>>>>
>>>>>>> @@ -81,17 +82,24 @@
>>>>>>>  bool operator!=(const TrackingMDRef &X) const { return MD != X.MD; }
>>>>>>>
>>>>>>> private:
>>>>>>> +  /// Enabler for DenseMapInfo of TrackingMDRefs, ensure the MD is non-null
>>>>>>> +  /// and not a special DenseMapInfo key.
>>>>>>
>>>>>> I don't think `TrackingMDRef` is appropriate as a `DenseMap<>` key.  Its
>>>>>> value can change to another arbitrary value.  If that happens, the
>>>>>> associated `DenseMapEntry` will have the wrong hash, and the table will
>>>>>> be invalid.
>>>>>>
>>>>>> As I mentioned above, I was reading the patch from the bottom, so I'm
>>>>>> not quite sure where you're actually using this... but I'd be happy to
>>>>>> discuss other possible solutions.
>>>>>
>>>>> I remember thinking I wanted to record the TrackingMDRef, but looking
>>>>> through the code I think things changed enough that I just want/need
>>>>> the Metadata pointer. I am not running into issues because nothing is
>>>>> changing on the TrackingMDRef between when I record it and use it. But
>>>>> that also points to not needing the TrackingMDRef. Will take a look at
>>>>> changing this part, along with addressing Mehdi's comments.
>>>>
>>>> Turning this into normal Metadata* instead of TrackingMDRef works
>>>> fine.
>>>
>>> In that case, please `assert(N.isResolved())` on any node that gets
>>> inserted into the set.
>>
>> I can't do that as some of these are temporary metadata. E.g. in the
>> first pass through when we are importing functions, this captures all
>> the metadata that is still temporary after mapping instructions but
>> not metadata. It is recording the mapping between those temporary
>> metadata and their value ids in the bitcode so that in the metadata
>> mapping postpass we can hook them up to the final non-temp values.
>>
>
> If it's not resolved, then it's not a good candidate for the key of
> a map.  Nodes that aren't resolved can change address when they get
> resolved.
>   - If you use TrackingMDRef, the key will change and the map will be
>     invalid.
>   - If you use Metadata*, the key will not change and the pointer will
>     be garbage.
>
> AFAICT, this DenseMap won't work.

I have to keep track of the temporary metadata left on the
instructions after function importing and before metadata linking. The
only time we have temporary metadata as a key in this map is well
before we will resolve it. At no point do we have a temporary metadata
key in the MDValueToValIDMap that is replaced with a non-temp value.
Let me explain what I'm doing and hopefully it will make more sense.

There are now two cases when we invoke the ModuleLinker to link in the module:
A) Function importing: import a single function, module-level bitcode
is not parsed/materialized.
B) Module linking post-pass: only module-level bitcode
parsed/materialized and mapped.

There are two maps used to support this:

1) DenseMap<const Metadata *, unsigned> &MDValueToValIDMap
- Populated from the BitcodeReader's MDValueList after parsing is complete
- In the case of A) it holds temporary metadata created when parsing
instructions for the imported function, since we don't map the
module-level metadata
- In the case of B) it holds non-temporary resolved metadata nodes for
the parsed module-level metadata
- It is used and then discarded after mapping in values, and in the
case of A where it contains temporary metadata, there is no
replacement of the temporary metadata with resolved non-temp metadata,
since it hasn't been materialized.

2) DenseMap<unsigned, MDNode *> *ValIDToTempMDMap
- Caller passes in a non-null value when we are either importing (A)
or linking metadata (B)
- Populated when mapping instructions from imported functions in A)
using the values in the above MDValueToValIDMap: When we try to map a
temporary metadata reached via an instruction, the new
ModuleLinker::mapTemporaryMetadata callback is invoked to find the
value index of the temporary metadata in MDValueToValIDMap, and record
the value index to temporary metadata mapping in ValIDToTempMDMap.
- Consumed when linking metadata (B), to replace temporary metadata
left on the imported instructions with the final non-temp metadata
that has the same value id: When we map to a non-temporary metadata
node in the value mapper, the new
ModuleLinker::replaceTemporaryMetadata callback is triggered, if the
non-temporary metadata's associated value index (in the
MDValueToValIDMap) has an entry in the ValIDToTempMDMap passed in for
this module, then the associated temporary metadata node is replaced
with the non-temporary resolved node, and the entry in the
ValIDToTempMDMap is erased. Note that for B, the MDValueToValIDMap
does *not* contain any temporary metadata.

Let me know if that clarifies why I have temporary metadata in a map,
or if you still have concerns.

Thanks,
Teresa




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