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

[Manman Ren]

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/4d54e037/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: 11171 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130903/4d54e037/attachment.obj>