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

[Manman Ren]

On Thu, Sep 5, 2013 at 10:10 AM, David Blaikie <dblaikie at gmail.com> wrote:

>
>  The comment added "getClassType()" can probably be omitted. It documents
>>> DITypeRef's semantics (& implementation details I mentioned weren't really
>>> public documentation for the type, let alone functions that return objects
>>> of that type) which are out of place here. We won't want to add such a
>>> documentation comment to every function that returns or handles a DITypeRef
>>> - we know what they are or we can look at the implementation (& comments,
>>> doc or otherwise) to find out, rather than redundantly documenting it at
>>> every use.
>>>
>>
>> I forgot to update the comments after wrapping Value* in DITypeRef.
>> Updated in the attached patch.
>>
>
> I'd still skip the "as a DITypeRef" - that's obvious from the signature,
> there's no need to duplicate that information in the comment. And if
> "ClassType" is unclear and "base type" is more clear, perhaps we should
> just rename the function so its purpose is obvious from the name & does not
> require a comment at all?
>
Updated the comments again:
/// The class type pointer-to-member is pointing to members of.


>
>
>
>>> Is there any reason to build the map of all the types in /all/ the CUs?
>>> rather than just building up a map for each CU as we visit it? (in the
>>> scope of the for loop a few lines below the "generateDITypeIdentifierMap"
>>> call)
>>>
>>  I don't see a for loop below the "generateDITypeIdentifierMap" call. Are
>> you referring to the implementation of generateDITypeIdentifierMap :)
>> When I search for "for (", they are all in the body of
>> generateDITypeIdentifierMap.
>>
>
> The for loop proceeding the call to generateDITypeIdentifierMap:
>
> "  // Emit initial sections so we can reference labels later.
>   emitSectionLabels();
>
>   for (unsigned i = 0, e = CU_Nodes->getNumOperands(); i != e; ++i) {"
>
Oh, I was checking the patch only. To me, it seems to be cleaner to have a
simple call to construct the Map, since the Map is shared across CUs.
Also if we have a def in one CU and a declaration in another, we prefer the
def. The earlier traversed CU can use the def if we build the Map in one
call.


>
>>
>>
>>> Why does DIDescriptor befriend DITypeRef?
>>>
>> If we don't, DITypeRef can't befriend the protected member function of
>> DIDescriptor (DIDescriptor::getFieldAs).
>>
>
> Hmm, OK - can't think of anything particularly better to do there, I
> suppose. Maybe a comment at the friend to explain why it's befriended?
>
Yes, add comments.
    // Befriends DITypeRef so DITypeRef can befriend the protected member
    // function: getFieldAs<DITypeRef>.

Thanks,
Manman

>
>
>>
>> Thanks,
>> Manman
>>
>>>
>>>
>>> - David
>>>
>>>
>>>
>>>
>>>> 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/20130905/3f82f8c2/attachment.html>