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

[Manman Ren]

On Wed, Sep 4, 2013 at 4:19 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
>
> On Tue, Sep 3, 2013 at 1:54 PM, Manman Ren <manman.ren at gmail.com> wrote:
>
>>
>>
>>
>> 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.
>>
>
> Sure, but it wouldn't need to - getDescriptorField could be replaced by
> the getField utility function in DebugInfo.cpp... except it returns Value*
> and only DITypeRef can be constructed from a Value*, all the other DI* need
> a MDNode*. OK, so I see now why we need a specialization here.
>
>
>> 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?
>>
>
> Yep, seemed to work for me. Just put this:
>
>   friend DITypeRef DIDescriptor::getFieldAs<DITypeRef>(unsigned Elt) const;
>
> in DITypeRef, instead of the template friend you have there.
>
Yes, updated in the attached patch.


>
>
>> 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 {
>>
>
> Yes, I was referring to those comments.
>
Updated in the attached patch.

>
>
>>
>>> 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?
>>
>
> fieldIsTypeRef looks good. As you say, since this deals with looking up a
> particular field in an mdnode, there's not an immediate reuse with the
> DITypeRef ctor. I don't think it's so critical then. I see you've changed
> it already - that's fine.
>
>
>>
>>
>>>
>>> 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.
>>
>
> 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'm still thinking that "generateDITypeIdentifierMap" should return a
> DITypeIdentifierMap, rather than take one by const ref. It'll model the
> semantics more clearly (this doesn't append to the map, or shouldn't - it
> does all the work to build the map across all the CUs & should do just
> that/not be affected by the prior state of the map, etc). DenseMap has a
> move assignment operator that should keep the performance characteristics
> fairly similar to the ref-passing implementation.
>
Done in attached patch.

>
> DITypeRef::resolve has a few "return DIType(...);" that can be written as
> "return ...;" instead (due to the implicit DIType(const MDNode*) ctor that
> you're relying on in "return NULL;" anyway)
>
Done in attached patch.

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


> Why does DIDescriptor befriend DITypeRef?
>
If we don't, DITypeRef can't befriend the protected member function of
DIDescriptor (DIDescriptor::getFieldAs).

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/c0dd8580/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: 13846 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130905/c0dd8580/attachment.obj>