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

[Manman Ren]

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/20130903/aee148a0/attachment.html>