[llvm] r203983 - Debug info: Unique types before emitting them to DWARF, where applicable.

[Manman Ren]

On Sun, Mar 16, 2014 at 12:50 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> On Mar 16, 2014, at 11:35 AM, Manman Ren <manman.ren at gmail.com> wrote:
>
> >
> >
> >
> > On Sat, Mar 15, 2014 at 8:16 AM, Adrian Prantl <aprantl at apple.com>
> wrote:
> >
> > On Mar 14, 2014, at 6:43 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > > On Fri, Mar 14, 2014 at 6:37 PM, Adrian Prantl <aprantl at apple.com>
> wrote:
> > >>
> > >> On Mar 14, 2014, at 17:59, David Blaikie <dblaikie at gmail.com> wrote:
> > >>
> > >>> On Fri, Mar 14, 2014 at 4:08 PM, Adrian Prantl <aprantl at apple.com>
> wrote:
> > >>>> Author: adrian
> > >>>> Date: Fri Mar 14 18:08:29 2014
> > >>>> New Revision: 203983
> > >>>>
> > >>>> URL: http://llvm.org/viewvc/llvm-project?rev=203983&view=rev
> > >>>> Log:
> > >>>> Debug info: Unique types before emitting them to DWARF, where
> applicable.
> > >>>>
> > >>>> Modified:
> > >>>>   llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
> > >>>>   llvm/trunk/test/Linker/type-unique-odr-a.ll
> > >>>>
> > >>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
> > >>>> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp?rev=203983&r1=203982&r2=203983&view=diff
> > >>>>
> ==============================================================================
> > >>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp (original)
> > >>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp Fri Mar 14
> 18:08:29 2014
> > >>>> @@ -958,6 +958,9 @@ DIE *DwarfUnit::getOrCreateTypeDIE(const
> > >>>>  DIE *ContextDIE = getOrCreateContextDIE(Context);
> > >>>>  assert(ContextDIE);
> > >>>>
> > >>>> +  // Unique the type. This is a noop if the type has no unique
> identifier.
> > >>>> +  Ty = DIType(resolve(Ty.getRef()));
> > >>>
> > >>> While this might be a good catch-all - I'm confused/concerned about
> > >>> what this implies. How did we get here? We must have some code
> > >>> referencing this type by node rather than name, yes? We should
> > >>> probably find what that is and fix it because that would mean the
> type
> > >>> won't be metadata node uniqued properly...
> > >>>
> > >>> Might be worth just turning this into an assert instead so we can
> find
> > >>> bugs like this more quickly if they arise.
> > >>
> > >> [+Manman]
> > >>
> > >> Interesting. It appears that the only callers of DIScope::getRef()
> are in DIBuilder, which explains why before this change, no ODR/name-based
> uniquing happened during LTO. (Only fully identical MDNodes would be
> uniqued).
> > >
> > > That's generally the idea - I don't think we thought that actually ODR
> > > uniquing would be needed and just deduplicating by structural
> > > equivalence (once the cycles were broken by using the mangled names
> > > instead of MDNode references) would be sufficient to minimize the
> > > debug info size to something acceptable.
> > >
> > >> What would be a more natural place to put this? AsmParser?
> > >
> > > Can we not just fix the DIScopes to use refs instead of MDNode*? I'm
> > > not sure what you're thinking we would put in AsmPrinter.
> >
> > Looking at the test case again, it looks like the problem is actually
> the list of retained types in the module  that doesn't use DIScopeRefs. I
> will see if that makes the problem disappear to a point were we can make
> the above code into an assertion as you suggested.
> >
> > We have a few places that we currently do not use DIRef: the list of
> retained types, scope for DIVariable, and types used in a DIArray ( I need
> to double check on that, we may have fixed it).
> >
> > I am going to add a FIXME to DIVariable.
> >
> > For the list of retained types, we have to use MDNodes instead of
> DIRefs, because we derive the map from type identifiers to MDNodes using
> the retained types.
>
> Another reason why we need to store the MDNodes in the retained types is
> that otherwise those types would be eliminated because no other MDNode is
> referencing them.
>
> > The correct way, I think, is to check if we have already visited a type
> identifier from the retained types to avoid visiting the same type
> identifier again.
>
> You mean, perform the lookup only when DwarfDebug is iterating over the
> retained types to emit them (instead of doing it for every type, as in
> r203983)?
>

Yes, when visiting the retained list of each CU, we don't want to generate
DIEs for DITypes with the same type identifier.

Cheers,
Manman


>
> thanks!
> adrian
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140316/a3b27c00/attachment.html>