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

[David Blaikie]

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.


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


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

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)

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)

Why does DIDescriptor befriend DITypeRef?

- 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/20130904/a37236bf/attachment.html>