[PATCH] First set of patches for type uniquing

Eric Christopher echristo at gmail.com
Fri Aug 23 10:49:14 PDT 2013


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



More information about the llvm-commits mailing list