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

[Manman Ren]

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.

   * "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/20130830/a273e196/attachment.html>