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

[Teresa Johnson via llvm-commits]

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.

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

>
>> +    return getTemporary(getContext(), getLine(), getColumn(), getRawScope(),
>> +                        getRawInlinedAt());
>>    }
>>
>>    // Disallow replacing operands.
>>
>



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