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

[Teresa Johnson via llvm-commits]

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

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