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

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

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

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