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

[David Blaikie]

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