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

[Manman Ren]

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

Thanks,
Manman


> thanks,
> adrian
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140316/84ee0ac6/attachment.html>