[Type Uniquing Patch] Use identifier instead of MDNode for getClassType of ptr_to_member.

[Manman Ren]

Updated patch is attached. It should address the above comments.
Please review,

Thanks,
Manman



On Tue, Sep 3, 2013 at 1:54 PM, Manman Ren <manman.ren at gmail.com> wrote:

>
>
>
> On Tue, Sep 3, 2013 at 1:24 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> Thanks - few follow-on or outstanding issues:
>>
>> Does getFieldAs<DYTypeRef> require an explicit specialization? I
>> speculated about this on IRC, but it was only a speculation/guess, I
>> didn't mean to claim I knew it was necessary. Looking at the code, the
>> implementation of that specialization looks the same as the original
>> template so can probably be omitted.
>>
> getFieldAs used to handle subclass of DIDescriptor, it treats the field as
> MDNode and constructs a DIDescriptor from it.
> For DITypeRef, the field can be either a MDNode or a MDString, that is why
> getField instead of getDescriptorField is used
> for DITypeRef.
>
>
>>
>> But if we do need the specialization, could we befriend it (or even
>> just befriend the whole template) and make the DITypeRef ctor private?
>>
> Can we actually befriend a specialization where class DITypeRef is not a
> template?
> If we befriend the whole template, we also need to make DITypeRef a friend
> of DIDescriptor so DITypeRef
> can access the protected member "DIDescriptor::getFieldAs".
>
>
>>
>> Remove the "protected" section from DITypeRef - the member should be
>> private & that's the default in a class anyway.
>>
> Will do.
>
>>
>> The documentation comment for DITypeRef discusses implementation
>> details that aren't really important/relevant to the contract of the
>> class. I'd probably just say something like "abstracts over direct and
>> identifier-based metadata type references" or the like.
>>
> Are you referring to the comments above the class definition?
> +  /// A thin wrapper to represent reference to a DIType. It wraps around
> Value*,
> +  /// which can be either a MDNode or a MDString, in the latter, MDString
> +  /// specifies the type identifier.
> +  class DITypeRef {
>
>>
>> fieldIsMDStringOrMDNode's name is non-descript in that it describes
>> the implementation, not the purpose. "isTypeRef" might be
>> simpler/better (this could even be re-used for the assert in
>> DITypeRef's ctor if the function can be placed somewhere sufficiently
>> general - maybe even a static member function of DITypeRef)
>>
> I thought about that, the only difference is one handles a field of a
> MDNode,
> the other handles the actual Value.
> Should we introduce fieldIsTypeRef and isTypeRef?
>
>
>>
>> Rename "lookupType" to "resolve" for consistency, perhaps?
>> (problematic to have different verbs/names for the same concept)
>>
> Will do.
>
> Thanks again for the quick review.
> Manman
>
>
>>
>>
>>
>>
>> On Tue, Sep 3, 2013 at 12:54 PM, Manman Ren <manman.ren at gmail.com> wrote:
>> > Attaching updated patch that should handle the review requests.
>> >
>> > Thanks,
>> > Manman
>> >
>> >
>> > On Fri, Aug 30, 2013 at 3:13 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>> >>
>> >> On Fri, Aug 30, 2013 at 3:10 PM, Manman Ren <manman.ren at gmail.com>
>> wrote:
>> >> > Once again, thanks for the quick review :)
>> >> >
>> >> >
>> >> > On Fri, Aug 30, 2013 at 11:19 AM, David Blaikie <dblaikie at gmail.com>
>> >> > wrote:
>> >> >>
>> >> >> Looks generally reasonable.
>> >> >>
>> >> >> I take it you're using pointer_to_member's base type as the first
>> >> >> field example & we'll see similar, smaller patches (not adding any
>> >> >> infrastructure, just porting another field over) for each field from
>> >> >> here on out? It seems like a nice, small example - thanks for that.
>> >> >
>> >> > Yes, that is the plan.
>> >> >>
>> >> >>
>> >> >> lib/IR/DebugInfo.cpp
>> >> >>  - DIDerivedType::Verify
>> >> >>    * we should have a single function that does
>> >> >> "fieldIsMDNode/fieldIsMDString" because we're going to use that test
>> >> >> for every type field
>> >> >
>> >> > Will do.
>> >> >>
>> >> >>  - generateDITypeIdentifierMap
>> >> >>    * any reason this wouldn't be a ctor of DITypeIdentifierMap (or
>> at
>> >> >> least a member function)?
>> >> >
>> >> > The current patch does not wrap around  DITypeIdentifierMap, that is
>> why
>> >> > generateDITypeIdentifierMap
>> >> > is a helper function. If we use a class for DITypeIdentifierMap, the
>> >> > only
>> >> > member will be a DenseMap and
>> >> > it can have member functions for generating the map and looking up
>> >> > values.
>> >>
>> >> Ah, right - yeah, it's probably not quite worth a separate type, then.
>> >> Optionally - we could return a DITypeIdentifierMap by value rather
>> >> than passing one in to populate, if the initialization happens at the
>> >> point of construction to take advantage of NRVO (or if DenseMap has
>> >> move-related overloads in C++11 mode, that'd suffice too).
>> >>
>> >> >
>> >> >>    * "DICompositeType Ty = DICompositeType(x);" can be rewritten as
>> >> >> "DICompositeType Ty(x);"
>> >> >
>> >> > Yes, will make the change.
>> >> >
>> >> >>
>> >> >>    * map lookup and insertion should be combined, probably simply
>> like
>> >> >> this:
>> >> >>      pair<iterator, bool> p = Map.insert(make_pair(TypeId, Ty)); //
>> >> >> try to insert
>> >> >>      if (!p.second && !Ty.isForwardDecl()) // if we didn't insert
>> >> >> because the element was already there
>> >> >>        p.first->second = Ty; // replace whatever we had (declaration
>> >> >> or definition) with a definition
>> >> >>      (pseudocode, comments for illustration only)
>> >> >
>> >> > Good deal, one search of the map instead of two.
>> >> >>
>> >> >>
>> >> >> DwarfDebug.cpp:
>> >> >>   - lookupType
>> >> >>     * assert should have an explanatory string in it
>> >> >
>> >> > Yes.
>> >> >>
>> >> >>
>> >> >> 'getClassType' - this is going to become pervasive in every DI*
>> >> >> function that returns DIType, right? I think it might be helpful to
>> >> >> add an opaque type here so we're not just referring to any Value.
>> >> >> Either something totally opaque, a class that befriends the
>> >> >> DIDescriptor class (for construction) and the map (for lookup) &
>> >> >> otherwise has no public members except simple copying, etc. Or
>> perhaps
>> >> >> a PointerUnion of MDNode/MDString - this is unnecessary, of course,
>> >> >> since the Value hierarchy already knows which object it is, so
>> there's
>> >> >> no need to discriminate the pointer in actuality...
>> >> >
>> >> > I will think about this more and chat with you on IRC.
>> >> >
>> >> > Thanks,
>> >> > Manman
>> >> >
>> >> >>
>> >> >>
>> >> >> On Fri, Aug 30, 2013 at 10:40 AM, Manman Ren <manman.ren at gmail.com>
>> >> >> wrote:
>> >> >> > Also add a typedef for DITypeIdentifierMap and a helper
>> >> >> > generateDITypeIdentifierMap in DebugInfo.h. In DwarfDebug.cpp, we
>> >> >> > keep
>> >> >> > a DITypeIdentifierMap and call generateDITypeIdentifierMap to
>> >> >> > actually
>> >> >> > populate the map. DwarfDebug also has a helper function lookupType
>> >> >> > that
>> >> >> > finds the type MDNode for a given Value (either an identifier in
>> >> >> > MDString
>> >> >> > or the actual MDNode).
>> >> >> >
>> >> >> > Verifier is updated accordingly.
>> >> >> >
>> >> >> > 3 patches are attached:
>> >> >> > The first one is a simple refactoring to add getField to
>> DIDescriptor
>> >> >> > so
>> >> >> > we
>> >> >> > can use it to return Value* for getClassType.
>> >> >> >
>> >> >> > The second one is the actual change to DIBuilder and DwarfDebug
>> etc
>> >> >> > with
>> >> >> > an
>> >> >> > added testing case.
>> >> >> >
>> >> >> > The last one is to update testing cases in clang.
>> >> >> >
>> >> >> > Thanks,
>> >> >> > Manman
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130904/6a834fb7/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Debug-Info-Use-identifier-to-reference-DIType-in-bas.patch
Type: application/octet-stream
Size: 13956 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130904/6a834fb7/attachment.obj>