[llvm-dev] distinct DISubprograms hindering sharing inlined subprogram descriptions

[Duncan P. N. Exon Smith via llvm-dev]

A few disjoint thoughts; sorry they're so delayed (I skimmed the responses below, and I think these are still relevant/not covered elsewhere).

Firstly, why *should* DISubprogram definitions be distinct?  There were two reasons this was valuable (this was from before there was a cu: link).

- It helped to fix long-standing bugs in the IRLinker, where uniqued-DISubprograms in different compile units would coalesce.  Now that the Function <-> DISubprogram connection has been reversed, I'm not entirely sure it's still a necessary precaution.

- It broke a several uniquing cycles involving DISubprogram.  The existence of the uniquing cycles meant that affected DISubprograms weren't really uniqued, anyway (since we don't bother to solve graph isomorphism); and breaking the cycles sped up the IRLinker and reduced memory waste.  Making DISubprogram uniqued again would either have no real effect (because distinct nodes they reference aren't uniqued anymore either) or bring back uniquing cycles (which would have effects... but not the desired one).

But there ought to be a way to get coalescing in the right places.  We just need to ensure that the uniqued/coalesced parts of the graph:
- are acyclic, and
- reference only coalesced nodes (note that leaves can happily include the strangely-coalesced distinct DICompositeType beast I created).

I've been thinking idly whether we can/should break DISubprogram apart into uniqued (coalesce-able) and distinct parts.  Still being someone ignorant of DWARF itself (and CodeView, for that matter), I'm not entirely sure.  But...

Here's my understanding of the current uses of DISubprogram (where I'm using DISubprogramDecl for the usually uniqued declarations, and DISubprogramDefn for the always distinct definitions):

// !dbg
Function -> DISubprogram

// Declaration (optional, only for member functions).
DISubprogramDefn -> DISubprogramDecl

// Variables:
DISubprogramDefn -> MDNode -> DILocalVariable

// Terminator for scope chains:
DILocalVariable [-> DILocalScope [-> ...]] -> DISubprogramDefn
DILocation [-> DILocalScope [-> ...]] -> DISubprogramDefn
DICompositeType -> DISubprogramDefn // function-local type

// Elements (member functions):
DICompositeType -> MDNode -> DISubprogramDecl
DISubprogramDecl -> DICompositeType
DISubprogramDefn -> DICompositeType

And here are the changes I would consider:

1. Split DISubprogramDecl and DISubprogramDefn into different types.

2. Remove definition-specific fields from DISubprogramDecl.
- variables, a "dangerous" source of uniquing cycles.
- declaration, which it will never have.
- scopeLine, which it will never have.
- unit, which the Defn will have when it's relevant.

3. Make a DISubprogramDecl mandatory for DISubprogramDefn.

4. Remove redundant fields from DISubprogramDefn.
- name and linkageName are on the declaration.
- type is redundant.
- flags is likely redundant?

Nothing so far has made any real changes, I think.  DISubprogramDecl should coalesce nicely between modules though.

5. Migrate links to point at DISubprogramDecl instead of DISubprogramDefn.
- DILocalVariable scope chain.
- DILocation scope chain.
- DICompositeType scope.

I don't think this will create any uniquing cycles.  DISubprogramDecl won't point at any of these.  Also, DILocalVariable should start coalescing with each other. 

Since DISubprogramDefn is only referenced from the Function !dbg attachment, only one should survive function-coalescing.

One open question, since I don't know the backend code, is: can you emit the "correct" DWARF after step (5)?  I'm hoping you only need the Defn of the Function that has instructions referencing the DILocalVariable/DILocation.  In which case, we suddenly get a lot of coalescing.

> On 2016-Dec-15, at 10:54, David Blaikie <dblaikie at gmail.com> wrote:
> 
> Branching off from a discussion of improvements to DIGlobalVariable representations that Adrian's working on - got me thinking about related changes that have already been made to DISubprogram.
> 
> To reduce duplicate debug info when things like linkonce_odr functions were deduplicated in LTO linking, the relationship between a CU and DISubprogram was inverted (instead of a CU maintaining a list of subprograms, subprograms specify which CU they come from - and the llvm::Function references the DISubprogram, so if the llvm::Function goes away, so does the associated DISubprogram)
> 
> I'm not sure if this caused a regression, but at least seems to miss a possible improvement:
> 
> During IR linking (for LTO, ThinLTO, etc) these distinct DISubprogram definitions (& their CU link, even if they weren't marked 'distinct', the CU link would cause them to effectively be so) remain separate - this means that inlined versions in one CU don't refer to an existing subprogram definition in another CU.
> 
> To demonstrate:
> inl.h:
> void f1();
> inline __attribute__((always_inline)) void f2() {
>   f1();
> }
> inl1.cpp:
> #include "inl.h"
> void c1() {
>   f2();
> }
> inl2.cpp:
> #include "inl.h"
> void c2() {
>   f2();
> }
> 
> Compile to IR, llvm-link the result. The DWARF you get is basically the same as the DWARF you'd get without linking:
> 
> DW_TAG_compile_unit
>   DW_AT_name "inl1.cpp"
>   DW_TAG_subprogram #0
>     DW_AT_name "f2"
>   DW_TAG_subprogram
>     DW_AT_name "c1"
>     DW_TAG_inlined_subroutine
>       DW_TAG_abstract_origin #0 "f2"
> DW_TAG_compile_unit
>   DW_AT_name "inl2.cpp"
>   DW_TAG_subprogram #1
>     DW_AT_name "f2"
>   DW_TAG_subprogram
>     DW_AT_name "c2"
>     DW_TAG_inlined_subroutine
>       DW_TAG_abstract_origin #1 "f2"
> 
> Instead of something more like this:
> 
> DW_TAG_compile_unit
>   DW_AT_name "inl1.cpp"
>   DW_TAG_subprogram #0
>     DW_AT_name "f2"
>   DW_TAG_subprogram
>     DW_AT_name "c1"
>     DW_TAG_inlined_subroutine
>       DW_TAG_abstract_origin #0 "f2"
> DW_TAG_compile_unit
>   DW_AT_name "inl2.cpp"
>   DW_TAG_subprogram
>     DW_AT_name "c2"
>     DW_TAG_inlined_subroutine
>       DW_TAG_abstract_origin #0 "f2"
> 
> (note that only one abstract definition of f2 is produced here)
> 
> Any thoughts? I imagine this is probably worth a reasonable amount of savings in an optimized build. Not huge, but not nothing. (probably not the top of anyone's list though, I realize)
> 
> Should we remove the CU link from a non-internal linkage subprogram (& this may have an effect on the GV representation issue originally being discussed) and just emit it into whichever CU happens to need it first?
> 
> This might be slightly sub-optimal, due to, say, the namespace being foreign to that CU. But it's how we do types currently, I think? So at least it'd be consistent and probably cheap enough/better.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20161223/7874e136/attachment-0001.html>