[PATCH] First set of patches for type uniquing

[Manman Ren]

llvm DIBuilder change is in r189410.
DIBuilder will use NULL when the identifier is empty.

Thanks,
Manman


On Tue, Aug 27, 2013 at 3:19 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Tue, Aug 27, 2013 at 3:14 PM, Manman Ren <manman.ren at gmail.com> wrote:
> >
> >> > I attached a patch for updating testing cases earlier:
> >> > it basically changed from null to the actual mangled name.
> >> > One example:
> >> >
> >> > -// CHECK: [[C:![0-9]*]] = {{.*}} metadata [[C_MEM:![0-9]*]], i32 0,
> >> > metadata [[C]], null, null} ; [ DW_TAG_structure_type ] [C] {{.*}}
> [def]
> >> >
> >> > +// CHECK: [[C:![0-9]*]] = {{.*}} metadata [[C_MEM:![0-9]*]], i32 0,
> >> > metadata [[C]], null, metadata !"_ZTS1C"} ; [ DW_TAG_structure_type ]
> >> > [C]
> >> > {{.*}} [def]
> >>
> >> OK, so you're planning on committing dependent patches - the change to
> >> LLVM to /allow/ passing in names will be revision-locked to the change
> >> to Clang to pass in (all?) names & update the test cases to reflect
> >> them?
> >
> > That was my plan :)
> >>
> >>
> >> I'm not a huge fan of rev-locked changes like that, but I realize
> >> they're necessary in the current architecture (even if we did the
> >> extra step I'm describing - where we had the ability to pass in the
> >> parameters, but we didn't pass them in - but still changed to
> >> zero-length strings rather than null, we'd have to do a batch test
> >> update)
> >>
> >> This does make me lean a bit more towards having DIBuilder insert null
> >> rather than zero-length strings, then you can commit the DIBuilder
> >> change right now without any other changes depending on it & without
> >> breaking Clang.
> >
> >
> > Yeah, it is a nice way to break into small patches. I will do that and
> > commit the
> > DIBuilder change first.
> >
> >>
> >>
> >> Then you can slowly add support for passing names from Clang at each
> >> of the "createXXX" DIBuilder calls, one at a time, updating only those
> >> test cases it affects while also adding backend functionality
> >> incrementally along with the paired DIBuilder changes for the
> >> referring fields.
> >>
> >> This means we'll be in an intermediate state for a while where a type
> >> is referenced both by real metadata references and via the
> >> identifiers, depending on which field used to reference it, until
> >> we've migrated all the references over & we will magically start
> >> seeing type nodes being shared across Compile Units at that point
> >> because we will have broken the cycles.
> >>
> >> >
> >> >
> >> > By applying the 3 patches (clang, llvm, clang-test), I still see one
> >> > failure: CodeGenCXX/debug-info-template.cpp fails with error: cannot
> yet
> >> > mangle expression type CXXUuidoofExpr.
> >> >
> >> > This can be fixed by modifying ItaniumMangler
> >> > (CXXNameMangler::mangleExpression), it currently has
> >> > FIXME: invent manglings for all these.
> >>
> >> Presumably we can just avoid trying to mangle these & not provide
> >> unique names for them for now. I'm not sure how special such a special
> >> case would be.
> >
> >
> > The mangler does not have an API of saying whether it can be mangled. It
> > will
> > just throw an error. Are you referring to a helper function in
> CGDebugInfo
> > to check
> > whether it can be mangled?
>
> I haven't looked at your patch in detail, but presumably we have logic
> to detect whether something's publicly mangleable & only try to mangle
> then. So maybe that would catch the UUID case too, if we do it right
> (or maybe not, I don't know).
>
> In addition/alternatively, I wonder why that test case wasn't already
> failing to mangle the UUID, I would assume the mangled name of the
> non-type template parameters is needed for the template to be
> compiled. So what does it do with the UUID, I wonder...
>
> >
> > Thanks,
> > Manman
> >
> >>
> >>
> >> >
> >> > Thanks,
> >> > Manman
> >> >
> >> >>
> >> >> >
> >> >> >>
> >> >> >> I'm always on the fence about the issues around null versus empty
> in
> >> >> >> strings that can't represent both (such as StringRef), but not
> >> >> >> having
> >> >> >> to update the test cases again might be sufficient motivation to
> use
> >> >> >> null instead of "" for these cases (adding a conditional operator,
> >> >> >> or
> >> >> >> a trivial function, to produce a null value when the string is
> empty
> >> >> >> &
> >> >> >> otherwise use MDString::get).
> >> >> >>
> >> >> >> With test case updates or the change to use null values for empty
> >> >> >> strings, please go ahead and commit this patch.
> >> >> >>
> >> >> >> >
> >> >> >> >
> >> >> >> > Attaching patches to implement the above item:
> >> >> >> > One patch on llvm side to update DIBuilder interface:
> >> >> >> > createClassType, createStructType, createUnionType,
> >> >> >> > createEnumerationType,
> >> >> >> > and createForwardDecl will take an optional StringRef for clang
> to
> >> >> >> > pass
> >> >> >> > in
> >> >> >> > the unique
> >> >> >> > identifier.
> >> >> >> >
> >> >> >> > One patch on clang side to actually generate the unique
> identifier
> >> >> >> > and
> >> >> >> > pass
> >> >> >> > it
> >> >> >> > to DIBuilder:
> >> >> >> > 1> add getUniqueTagTypeName to use CXX mangler to generate a
> >> >> >> > unique
> >> >> >> > identifier for
> >> >> >> > external CXX TagType.
> >> >> >> > 2> call getUniqueTagTypeName and add the type to RetainedTypes
> in
> >> >> >> > a
> >> >> >> > few
> >> >> >> > places, before
> >> >> >> > creating a ClassType, StructType, UnionType, EnumerationType, or
> >> >> >> > ForwardDecl.
> >> >> >>
> >> >> >> Could we move type retention into DIBuilder so it could manage
> >> >> >> adding
> >> >> >> any type with a unique identifier to it automatically? (or at
> least
> >> >> >> allow the list of retained types handled by Clang to be appended
> to
> >> >> >> a
> >> >> >> list that DIBuilder mantains?)
> >> >> >
> >> >> >
> >> >> > That is another possibility, but we have to update
> >> >> > DIBuilder::AllRetainTypes
> >> >> > to be VH instead of
> >> >> > plain Value *, since the types created can later on be modified.
> The
> >> >> > patch
> >> >> > updates CGDebugInfo::RetainedTypes and
> >> >> > uses TypeCache to find the corresponding DIType in finalize(),
> there
> >> >> > it
> >> >> > calls DIBuilder.retainType to add to DIBuilder.AllRetainTypes.
> >> >>
> >> >> That sounds reasonable. A test case in Clang (since that's
> essentially
> >> >> how DIBuilder is tested, unfortunately) that would fail if this was
> >> >> implemented the way I suggested (using retainType directly from
> within
> >> >> createStructureType, etc, whenever the Unique Identifier is provided
> -
> >> >> but without making the Value*/VH change you mentioned is needed).
> >> >>
> >> >> >
> >> >> >>
> >> >> >>
> >> >> >> > 3> Input for getOrCreateRecordFwdDecl is modified from
> RecordDecl
> >> >> >> > to
> >> >> >> > RecordType to enable
> >> >> >> > CXX mangler, same applies for CreateEnumType.
> >> >> >>
> >> >> >> It's easy to get a RecordType from a RecordDecl (getTypeForDecl) &
> >> >> >> only slightly harder to go the other way - but if this actually
> >> >> >> simplifies code I'm OK with it, but would probably want this patch
> >> >> >> pulled out separately for ease of review.
> >> >> >
> >> >> > Sure, I can pull out the part separately.
> >> >> >
> >> >> >>
> >> >> >>
> >> >> >> > 4> It is possible that the same Type is pushed to RetainedTypes
> >> >> >> > multiple
> >> >> >> > times, so a SmallPtrSet
> >> >> >> > is used locally in finalize() to remove duplication.
> >> >> >>
> >> >> >> When does this happen? That sounds like it might be a bug in
> Clang,
> >> >> >> or
> >> >> >> at least something we could tidy up there to make this less
> >> >> >> complicated.
> >> >> >
> >> >> > I will look deeper at this and figure out why.
> >> >> >
> >> >> > Thanks for the quick review.
> >> >> >
> >> >> > Manman
> >> >> >>
> >> >> >>
> >> >> >> >
> >> >> >> > The 3rd patch to update testing cases on clang side:
> >> >> >> >
> >> >> >> > There is one remaining issue with the current implementation of
> >> >> >> > using
> >> >> >> > CXX
> >> >> >> > mangler
> >> >> >> > to generate the unique identifier:
> >> >> >> > CodeGenCXX/debug-info-template.cpp
> >> >> >> > fails
> >> >> >> > with error:
> >> >> >> > cannot yet mangle expression type CXXUuidoofExpr.
> >> >> >>
> >> >> >> > We can definitely fix the above error in ItaniumMangle.cpp, but
> I
> >> >> >> > want
> >> >> >> > to
> >> >> >> > see whether the approach I am
> >> >> >> > taking is on the right track.
> >> >> >> >
> >> >> >> > Thanks,
> >> >> >> > Manman
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130827/d4a6bc8b/attachment.html>