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

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

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