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

[Teresa Johnson via llvm-commits]

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.

>
>> I will update the patch with the modified version. I still need
>> to address Mehdi's comments, that will be in the next round.
>>
>>>
>>>>
>>>>> +  bool isValid() {
>>>>> +    return MD && MD != DenseMapInfo<Metadata *>::getEmptyKey() &&
>>>>> +           MD != DenseMapInfo<Metadata *>::getTombstoneKey();
>>>>> +  }
>>>>> +
>>>>>   void track() {
>>>>> -    if (MD)
>>>>> +    if (isValid())
>>>>>       MetadataTracking::track(MD);
>>>>>   }
>>>>>   void untrack() {
>>>>> -    if (MD)
>>>>> +    if (isValid())
>>>>>       MetadataTracking::untrack(MD);
>>>>>   }
>>>>>   void retrack(TrackingMDRef &X) {
>>>>>     assert(MD == X.MD && "Expected values to match");
>>>>> -    if (X.MD) {
>>>>> +    if (X.isValid()) {
>>>>>       MetadataTracking::retrack(X.MD, MD);
>>>>>       X.MD = nullptr;
>>>>>     }
>>>>> @@ -165,6 +173,22 @@
>>>>>   }
>>>>> };
>>>>>
>>>>> +/// Provide DenseMapInfo for TrackingMDRefs.
>>>>> +template <> struct DenseMapInfo<TrackingMDRef> {
>>>>> +  static inline TrackingMDRef getEmptyKey() {
>>>>> +    return TrackingMDRef(DenseMapInfo<Metadata *>::getEmptyKey());
>>>>> +  }
>>>>> +  static inline TrackingMDRef getTombstoneKey() {
>>>>> +    return TrackingMDRef(DenseMapInfo<Metadata *>::getTombstoneKey());
>>>>> +  }
>>>>> +  static unsigned getHashValue(const TrackingMDRef Val) {
>>>>> +    return DenseMapInfo<Metadata *>::getHashValue(Val.get());
>>>>> +  }
>>>>> +  static bool isEqual(const TrackingMDRef LHS, const TrackingMDRef RHS) {
>>>>> +    return LHS == RHS;
>>>>> +  }
>>>>> +};
>>>>> +
>>>>> } // end namespace llvm
>>>>>
>>>>> #endif
>>>>> Index: include/llvm/IR/DebugInfoMetadata.h
>>>>> ===================================================================
>>>>> --- include/llvm/IR/DebugInfoMetadata.h
>>>>> +++ include/llvm/IR/DebugInfoMetadata.h
>>>>> @@ -1118,8 +1118,11 @@
>>>>>   }
>>>>>
>>>>>   TempDILocation cloneImpl() const {
>>>>
>>>> This seems possible to split out and test with a C++ unit test.
>>>
>>> Ok.
>>>
>>>>
>>>>> -    return getTemporary(getContext(), getLine(), getColumn(), getScope(),
>>>>> -                        getInlinedAt());
>>>>> +    // Get the raw scope/inlinedAt since it is possible to invoke this on
>>>>> +    // a DILocation containing temporary metadata. Specifically, this happens
>>>>> +    // when we are linking metadata as a postpass after function importing.
>>>>
>>>> I think this comment belongs in the commit message, as once the code is
>>>> committed we'll grow more uses of it.  The C++ unit test should be
>>>> enough to prevent tampering.
>>>
>>> Will do. I am thinking of leaving the first sentence (to explain why
>>> this code is different from the rest of the subclasses which use
>>> getScope, but will remove the second.
>>
>> I have this part ready with a unittest. Do you want to review that
>> separately, or should I just commit it?
>
> Up to you; seems like a candidate for post-commit review to me.

Ok will do.

Thanks,
Teresa

>
>>>
>>>>
>>>>> +    return getTemporary(getContext(), getLine(), getColumn(), getRawScope(),
>>>>> +                        getRawInlinedAt());
>>>>>   }
>>>>>
>>>>>   // Disallow replacing operands.
>>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
>>
>>
>>
>> --
>> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
>



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