[PATCH] First set of patches for type uniquing
Manman Ren
manman.ren at gmail.com
Mon Aug 26 13:20:40 PDT 2013
Hi David,
Thanks for the fast reviewing.
On Mon, Aug 26, 2013 at 12:58 PM, David Blaikie <dblaikie at gmail.com> wrote:
> Generally we include test changes in patches that require them, rather
> than splitting them out. I assume in this instance you split the
> patches for ease of reviewing, but intend to commit them together (so
> as not to break the buildbots)
>
Yes, I separated to make reviewing easier. The three patches will be
submitted together.
>
> It looks like we're constraining the DICompositeType schema a little
> further than it was before - as you can see from your patch, some
> DICompositeTypes had 13 fields (no template argument field) and some
> have 14. Now they all have 15. I think this is OK, but just want to
> highlight it as a change in schema.
Yes, all DICompositeTypes will have 15 fields, for types where template
argument field was omitted, I
added an extra null. I will expand my commit message to emphasize the
schema change.
> (personally I don't like the vague
> optionality of trailing fields like that - if we're going to try to
> reduce the size of these records (there is a lot of redundancy in them
> - some type descriptions don't need nearly so many fields) we should
> sit down & do that deliberately rather than with little hacks, so
> making things more consistent is good as it'll make it easier to
> decide where to try to remove such redundancy)
>
> While you're here, you could change those lines that use
> "Constant::getNullValue" to just use NULL as we do in other places (I
> assume the "getNullValue" is how we end up with i32 0 as 'null' values
> sometimes?
Yes, that is how we get "i32 0".
> when we really want null values anyway, and it's shorter to
> write) such as in createSubroutineType, createEnumerationType,
> createArrayType - just on the lines you're already touching (because
> you have to add a ',') anyway.
I will make the change.
Thanks,
Manman
>
> But otherwise this looks correct - Please commit. (I haven't tested
> this, so leaving it up to you to provide assurances that your test
> changes are necessary & sufficient - I glanced at a few, but didn't
> rigorously investigate them all).
>
> On Mon, Aug 26, 2013 at 12:50 PM, Manman Ren <manman.ren at gmail.com> wrote:
> >
> > 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/8b9110a0/attachment.html>
More information about the llvm-commits
mailing list