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

[David Blaikie]

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.

But if we do need the specialization, could we befriend it (or even
just befriend the whole template) and make the DITypeRef ctor private?

Remove the "protected" section from DITypeRef - the member should be
private & that's the default in a class anyway.

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.

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)

Rename "lookupType" to "resolve" for consistency, perhaps?
(problematic to have different verbs/names for the same concept)




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