[PATCH] First set of patches for type uniquing

Manman Ren manman.ren at gmail.com
Mon Aug 26 12:50:22 PDT 2013


Eric, David and I chatted on IRC and agreed to submit smaller patches.
We will add an identifier field to DICompositeType. For DICompositeTypes
with a non-null identifier field, its reference can
be replaced with the identifier to remove cycles and to help type uniquing.

As a first step, I am going to add a field to DICompositeType, the attached
patches implement that:
DICompositeType will have an identifier field at position 14. For now, the
field is set to null by DIBuilder.
Update verifier to check that DICompositeType has 15 fields and the last
field is null or a MDString.
Update testing cases to include an extra field for DICompositeType.
The identifier field will be used by type uniquing so a front end can
generate a DICompositeType with a unique identifier.

3 patches are attached, two of them update testing cases.

Thanks,
Manman



On Fri, Aug 23, 2013 at 10:49 AM, Eric Christopher <echristo at gmail.com>wrote:

> >>
> >> I'm still looking at the patch but one comment that will affect a lot
> >> of the text of the patch is that I think the unique name should be
> >> passed into the type creation functions, not created inside the
> >> backend. This means that each front end can use the definition of
> >> "unique" that most appeals to them.
> >
> >
> >
> > I know dragonegg calls those type creation functions, and there are maybe
> > other out-of-tree usages.
> > This patch provides default value for the unique name (an empty string)
> to
> > the type creation functions, so
> > it is not going to break dragonegg or out-of-tree projects. Inside the
> type
> > creation functions, if the passed-in
> > unique name is empty, DIBuilder will generate a unique name. I am not
> sure
> > if it is necessary for dragonegg
> > to have its own implementation of generating the unique name, and how
> hard
> > it is.
>
> It shouldn't be too hard, the question is what should go into the
> field. I don't know how comfortable I am with emitting a "unique" name
> out of the backend if one isn't provided. I think the best method here
> would be this:
>
> The type field can either be an MDString or an MDNode. The MDNode will
> be the referenced type as is currently used. The MDString would be the
> unique name for the type that will be used to disambiguate as we've
> discussed (also to remove cycles and references so we can unique).
> This will avoid the need to change clients except for those that wish
> to take advantage of this.
>
> Here's a very incremental way of doing the patch I think:
>
> 1) Add a field to DIComposite type which is initially just a direct
> reference back to itself.
> 2) Ensure that these end up in the retain types list.
> 3) Then build the map based on that key, which is just a node
> reference at the moment.
>  - Each field can thus be done incrementally and mapped one at a time.
> 4) Then start passing in strings one at a time.
>
> This will make it easier to review etc.
>
> Then we can get to the declaration deduplication and
> declaration/defined type merging.
>
> What do you think?
>
> >> Also I'm not sure why you've got an actual hashing algorithm in there.
> >> Can you explain?
> >
> >
> > Which hashing algorithm are you referring to?
> > I don't recall a hashing algorithm in the patch.
> >
>
> +    static unsigned getHashValue(const Pair& PairVal) {
> +      uint64_t Key = (uint64_t)hash_value(PairVal.first) << 32
> +                     | (uint64_t)PairVal.second;
> +      Key += ~(Key << 32);
> +      Key ^= (Key >> 22);
> +      Key += ~(Key << 13);
> +      Key ^= (Key >> 8);
> +      Key += (Key << 3);
> +      Key ^= (Key >> 15);
> +      Key += ~(Key << 27);
> +      Key ^= (Key >> 31);
> +      return (unsigned)Key;
> +    }
>
> Related to the many pairs that we talk about below.
>
> >> Couple other things I noticed:
> >>
> >> +  // If the consumer of DIBuilder does not provide a unique name,
> >> generate one
> >> +  // here.
> >> +  MDString *UniqueStr = UniqueName.empty() ?
> >> +                        getUniqueTypeName(dwarf::DW_TAG_structure_type,
> >> Name) :
> >> +                        MDString::get(VMContext, UniqueName);
> >>
> >> I don't think we should do this. Have an example where we'd want to?
> >
> >
> > The patch has a default value of an empty string for the type creation
> > function, so if the caller of the type
> > creation function does not provide a name, DIBuilder will come up with
> one.
>
> Yeah, not such a fan. Either we should do this in such a way that we
> don't require outside clients to update, or decide that we don't care.
>
> >> +        Map.insert(std::make_pair(
> >> +                   std::make_pair(Ty.getUniqueName(),
> >> Ty.isForwardDecl()),
> >> +                   Ty));
> >>
> >> A pair of a pair is a bit icky here as well.
> >
> > Any suggestion here?
> >
>
> I'm not sure what each field is being used for in the map so I can't
> provide suggestions. Perhaps you can explain what's going on? (The
> StringRef and the DIType are obvious, the bool not so much.)
>
> -eric
>
> > Thanks,
> > Manman
> >
> >>
> >>
> >> This should be enough to start discussion/revision of the patch.
> >>
> >> -eric
> >>
> >> On Wed, Jul 31, 2013 at 4:37 PM, Manman Ren <manman.ren at gmail.com>
> wrote:
> >> >
> >> > After cleaning up usage of the verify function and all the testing
> >> > cases,
> >> > the first set of patches are ready for review :)
> >> >
> >> > 3 patches are included: one for clang (tu_clang_07312013.patch), one
> for
> >> > code change of llvm (tu_07312013.patch), and one for updating all the
> >> > testing cases (tu_test_07312013.patch).
> >> >
> >> > The following are implemented:
> >> > 1> DIType will have a unique name at field 9.
> >> >      For DIBasicType, DICompositeType, DIDerivedType, the fields after
> >> > will
> >> > be shifted by 1.
> >> > 2> I am going to update usage of DIType in phases:
> >> >      At the first batch, getContainingType of DICompositeType and
> >> > DISubprogram will be replaced with a MDString, as well as getClassType
> >> > of
> >> > DIDerivedType.
> >> > 3> The verifier is updated according to 1 and 2.
> >> > 4> DwarfDebug has a TypeMap.
> >> >       It also has a helper function lookupType to find the
> corresponding
> >> > DIType given a type name.
> >> > 5> how to generate the unique name?
> >> >       The clang frontend has two helper functions:
> getUniqueTagTypeName
> >> > and
> >> > getUniqueTypeName
> >> >           The first one handles TagType and it uses ODR (One
> Definition
> >> > Rule) for external c++ types. It is currently called for struct,
> class,
> >> > union and enum.
> >> >           The second one can be used for general types, it uses
> mangled
> >> > name
> >> > appended with CU's directory and file name to provide uniqueness
> across
> >> > CUs.
> >> > It is currently used to generate unique name for subroutine type. For
> >> > other
> >> > types, DIBuilder will provide a unique name. Clang can handle more
> types
> >> > in
> >> > follow-on patches if it can provide a better name than DIBuilder.
> >> >       DIBuilder also has a helper function getUniqueTypeName, this is
> >> > called
> >> > when consumer of DIBuilder does not provide a unique type name. This
> is
> >> > needed for dragonegg, before it provides a unique name (not sure
> whether
> >> > it
> >> > is needed).
> >> > 6> All DITypes created by DIBuilder will be added to RetainTypes.
> >> >
> >> > Comments and feedback are welcome.
> >> >
> >> > Manman
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130826/5fe33956/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Debug-Info-add-an-identifier-field-to-DICompositeTyp.patch
Type: application/octet-stream
Size: 6380 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130826/5fe33956/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Type-Uniquing-update-testing-cases.patch
Type: application/octet-stream
Size: 523642 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130826/5fe33956/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Auto-update-2.patch
Type: application/octet-stream
Size: 22803 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130826/5fe33956/attachment-0002.obj>


More information about the llvm-commits mailing list