[llvm] r191792 - Debug Info: remove duplication of DIEs when a DIE is part of the type system

[David Blaikie]

On Fri, Oct 4, 2013 at 11:38 AM, Manman Ren <manman.ren at gmail.com> wrote:

>
>
> On Fri, Oct 4, 2013 at 9:41 AM, Eric Christopher <echristo at gmail.com>wrote:
>
>> Hi Manman,
>>
>> In reviewing this code I've found a few problems with how you're
>> emitting the debug info for ref_addr.
>
>
>
>
>> It needs to be a relocatable
>> value from the beginning of the section and while it's a reasonably
>> straightforward change it is going to require some work to plumb I
>> think.
>
>
> The issue seems to be related to handling of ref_addr in general.
> So we might have problem even after this commit is reverted.
>
> ref_addr currently uses the offset from beginning of the debug_info
> section, and the following code has been there since r176882 (I added it
> in, so it is still my bug :)
>       unsigned Addr = Origin->getOffset();
>       if (Form == dwarf::DW_FORM_ref_addr) {
>         // For DW_FORM_ref_addr, output the offset from beginning of debug
> info
>         // section. Origin->getOffset() returns the offset from start of
> the
>         // compile unit.
>         DwarfUnits &Holder = useSplitDwarf() ? SkeletonHolder : InfoHolder;
>         Addr += Holder.getCUOffset(Origin->getCompileUnit());
>       }
> Of course, this commits start to use ref_addr much more frequently.
>

Agreed. But without /any/ test coverage it's hard to guess how this may or
may not break things. We should go back and have a discussion about that
change (I replied to the original commit to restart the conversation there).


>
> With this and the design discussions we've been going through
>> with it I'm going to revert the patch for now and get you a testcase.
>>
>
> Did we reach any conclusion on the design discussions? It seems to me we
> should not gate this on implementation of type units.
>

As I've said on this thread and IRC, I don't intend to gate this on type
units.


> And this should be implemented in a similar way as this commit.
>

We've bees discussing the design, I think it'll be easier to discuss with
an actual patch review. Please work up a patch that addresses, as best as
you understand it, the design concerns we've discussed in this thread and
the bug shown, and send it out for pre-commit review.


> What other concerns do you have?
>
> Thanks,
> Manman
>
>
>> This is a good idea for work and is something I definitely want us to
>> pursue in the long run (or immediate term depending on your
>> priorities) so don't look at this as anything more than me needing to
>> revert due to a bug.
>>
>> Thanks!
>>
>> -eric
>>
>>
>> On Thu, Oct 3, 2013 at 8:50 PM, Manman Ren <manman.ren at gmail.com> wrote:
>> >
>> >
>> >
>> > On Thu, Oct 3, 2013 at 4:24 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>> >>
>> >>
>> >>
>> >>
>> >> On Thu, Oct 3, 2013 at 4:10 PM, Manman Ren <manman.ren at gmail.com>
>> wrote:
>> >>>
>> >>>
>> >>>
>> >>>
>> >>> On Thu, Oct 3, 2013 at 2:06 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>> >>>>
>> >>>>
>> >>>>>> 3GB memory reduction for xalan built with lto -g (without removing
>> DIE
>> >>>>>> duplication, the memory usage is 7GB)
>> >>>>>
>> >>>>>
>> >>>>> OK, so you're looking at memory usage during LTO, not the size of
>> the
>> >>>>> resulting debug info? (that's fine, just trying to understand
>> exactly what
>> >>>>> metrics you're targeting)
>> >>>>
>> >>>>
>> >>>> One thought I had (and just discussed with Eric - he seemed to think
>> it
>> >>>> was plausible, at least):
>> >>>>
>> >>>> If you're interested in reducing peak memory usage of LLVM during
>> LTO,
>> >>>> have you considered modifying debug info generation not to cache all
>> the
>> >>>> compile_units before emitting them? If we emitted one CU at a time
>> as they
>> >>>> were created I imagine the memory footprint would be much lower.
>> >>>
>> >>>
>> >>> Yes, that is on my todo list after type uniquing is done.
>> >>
>> >>
>> >> So, as I said, I think it might be better to do that work first. It may
>> >> have a substantial impact on cross-CU references. (see below)
>> >>
>> >>>
>> >>> Right now, we don't release any memory and we have layers of memory
>> usage
>> >>> related to debug info: MDNodes, DIEs and some data in MC layer.
>> >>>  All these layers exist for the whole program with LTO.
>> >>>
>> >>>>
>> >>>> (and we could still, potentially, add the type cross-referencing by
>> just
>> >>>> caching the section offsets or labels, etc, of the types emitted in
>> prior
>> >>>> units - though that wouldn't be perfect for types that contain some
>> members
>> >>>> (implicit special members) in one CU and a different set of members
>> in
>> >>>> another CU (and it'll be a bit trickier for type units - we'd need
>> to cache
>> >>>> any subgraph of potentially self-referencing type units until all the
>> >>>> signatures are resolved, then they could all be emitted))
>> >>>>
>> >>>> This isn't necessarily a place you should start with - I can
>> appreciate
>> >>>> that your current approach is probably higher value to you for lower
>> >>>> engineering cost (and thus gets you closer sooner) but if it's still
>> not
>> >>>> going to be sufficient for your needs, especially if you're going to
>> have to
>> >>>> implement something like emit-CU-at-a-time as I describe above
>> anyway, it
>> >>>> might be more valuable to lay that foundation first.
>> >>>
>> >>>
>> >>> Even if we emit one CU at a time, it is still good to share the types
>> >>> across-CU so we don't waist time constructing the type DIEs for each
>> CU, and
>> >>> this approach (remove DIE duplication) also reduces the raw DWARF
>> size.
>> >>
>> >>
>> >> I agree, from an output size perspective we'll still want to share DIEs
>> >> across CUs - but I think you might see bigger memory footprint (since
>> that's
>> >> the stat you're aiming at at the moment) wins from this change, but
>> more
>> >> importantly - I think a change like that will have a significant
>> impact on a
>> >> feature like the cross-CU DIE referencing you're doing here.
>> >>
>> >> If we emit one CU at a time, cross-CU DIE references will look very
>> >> different - it won't be simply a matter of getting a DIE from the
>> cross-CU
>> >> map anymore. Likely we'll want to store a much more minimal form of
>> the DIE
>> >> (just it's section offset or label) in a table once the prior CU has
>> been
>> >> emitted. Then when a future CU is emitted and wants to reference that,
>> we
>> >> can lookup that remnant in the table - the whole DIE won't exist
>> anymore.
>> >
>> >
>> > With "emit-a-CU-at-a-time", we can still keep the type DIEs around since
>> > they are going to be shared across CUs.
>> > We can also store a minimal form of the type DIEs to further reduce the
>> > memory footprint.
>> >
>> > I agree that we have to change the cross-CU references when we only
>> store a
>> > minimal form of the type DIEs. But the change should
>> > not be complicated, we now store a map from MDNode to DIE, and that can
>> be
>> > changed to map from MDNode to MinimalDIE.
>> >
>> > "emit-a-CU-at-a-time" is a known solution to reduce memory footprint,
>> but I
>> > don't have a time line for when I will work on it.
>> >
>> >>
>> >>
>> >> It seems like the bigger change should go first so we don't avoid
>> >> discussing a lot of this design only to have to rework it
>> substantially when
>> >> we emit a CU-at-a-time.
>> >
>> > Since we already did a lot of discussion about this design and it is not
>> > complicated (about 30 to 40 lines of code), let's discuss
>> > "emit-a-CU-at-a-time" when it is somebody's immediate task.
>> >
>> > Thanks,
>> > Manman
>> >
>> >>
>> >>>
>> >>>
>> >>>>
>> >>>>
>> >>>> If not, I'd still probably like to discuss the design of the cross-CU
>> >>>> referencing work you have here to decide on the right design in a
>> normal
>> >>>> patch review process rather than trying to think about how the
>> current code
>> >>>> can be morphed into a better design.
>> >>>
>> >>>
>> >>> What other designs do you have in mind?
>> >>
>> >>
>> >> Just some of the stuff we've already been discussing such as merging
>> the
>> >> maps and simplifying the DwarfDebug entry points. But if you're already
>> >> planning to do CU-at-a-time, I wonder whether it's worth discussing the
>> >> design of cross-CU references before we do CU-at-a-time - it seems to
>> me
>> >> like the implementation would change quite significantly.
>> >>
>> >>>
>> >>>
>> >>> Manman
>> >>>>
>> >>>>
>> >>>>
>> >>>> - David
>> >>>
>> >>>
>> >>
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131004/f268ea7e/attachment.html>