[llvm] r246098 - DI: Make Subprogram definitions 'distinct'

Wed Aug 26 17:28:55 PDT 2015

> On 2015-Aug-26, at 17:01, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Wed, Aug 26, 2015 at 3:50 PM, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> Author: dexonsmith
> Date: Wed Aug 26 17:50:16 2015
> New Revision: 246098
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=246098&view=rev
> Log:
> DI: Make Subprogram definitions 'distinct'
> 
> Change `DIBuilder` always to produce 'distinct' nodes when creating
> `DISubprogram` definitions.  I measured a ~5% memory improvement in the
> link step (of ld64) when using `-flto -g`.
> 
> `DISubprogram`s are used in two ways in the debug info graph.
> 
> Some are definitions, point at actual functions, and can't really be
> shared between compile units.  With full debug info, these point down at
> their variables, forming uniquing cycles.  These uniquing cycles are
> expensive to link between modules, since all unique nodes that reference
> them transitively need to be duplicated (see commit message for r244181
> for more details).
> 
> *reads r244181*
> 
> This does seem a bit awkward that we'll need to mark any regularly-cycle-creating nodes as distinct for performance reasons... :/ (but I imagine there's not much else to be done about it - ceratinly it's easy for the nodes that obviously wanted distinct-ness to begin with, but those that just happened across it due to a relationship like the subprogram->variable one (we only emit the variable list in optimized mode though, don't we?) seem quirk-ily unfortunate)

I think everything in a uniquing cycle really wants a distinct node
somewhere.  As soon as they're in a cycle, they're not really
unique anyway.  IMO, a lot of the debug info graph doesn't make sense
to unique.  (For example, I think lib/Linker should be explicitly
doing type uniquing, and all DICompositeTypes with a UUID should be
'distinct'.)

Regarding subprogram definitions specifically: I don't think there's
a benefit to sharing them across compile units, because each
translation unit has its own definition of the associated functions.
The uniquing behaviour doesn't do anything but introduce bugs (like
the corner case of PR22792 I fixed in r233302, as I just discovered!
I'm working on updating testcases for the follow-up commit, and
realized that this change prevents that final lib/Linker bug up
front).

Eventually I want to stop loading/linking the subprograms if we don't
take their associated functions.

> Speaking of the unoptimized case (that doesn't have a variable list, if I recall correctly - but maybe we changed that) - is this worse in any way to mark these nodes as distinct? Did they get naturally deduplicated previously & now have to be explicitly de duplicated? Not sure.

No, they weren't naturally deduplicated.  The only way this makes more
metadata at -O0 is if someone is running `-O0 -g -flto`, a pretty
strange configuration.  Even there I don't see a problem.

> I guess at some point these will be attached to llvm::Functions directly, I think was the idea?

Exactly.  That's PR23367.  I'm hoping to do that next.  I thought it
might be easier to get right after making definitions 'distinct' (as
I had a vague recollection of the problems I had with PR22792).

> (though not sure how we'll make a CU list of subprograms/functions to generate)

That's already generated from the SPs that are actually referenced
from the code, but we'll still (at least temporarily) need a map from
CU => SP to know which CU each SP is in.  I'd like to replace that
with a pointer in the other direction (ideally a transitive pointer
through the SP's `scope:` chain).

> Not sure if that'll change/simplify/make this less odd... 

I hope so!

> Others are declarations, primarily used for member functions in the type
> hierarchy.  Definitions never show up there; instead, a definition
> points at its corresponding declaration node.
> 
> I started by making all subprograms 'distinct'.  However, that was too
> big a hammer: memory usage *increased* ~5% (net increase vs. this patch
> of ~10%) because the 'distinct' declarations undermine LTO type
> uniquing.  This is a targeted fix for the definitions (where uniquing is
> an observable problem).
> 
> A couple of notes:
> 
>   - There's an accompanying commit to update IRGen testcases in clang.
>   - ^ That's what I'm using to test this commit.
>   - In a follow-up, I'll change the verifier to require 'distinct' on
>     definitions and add an upgrade to `BitcodeReader`.
> 
> Modified:
>     llvm/trunk/lib/IR/DIBuilder.cpp
> 
> Modified: llvm/trunk/lib/IR/DIBuilder.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=246098&r1=246097&r2=246098&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/DIBuilder.cpp (original)
> +++ llvm/trunk/lib/IR/DIBuilder.cpp Wed Aug 26 17:50:16 2015
> @@ -675,6 +675,13 @@ DISubprogram *DIBuilder::createFunction(
>                          Flags, isOptimized, Fn, TParams, Decl);
>  }
> 
> +template <class... Ts>
> +static DISubprogram *getSubprogram(bool IsDistinct, Ts &&... Args) {
> +  if (IsDistinct)
> +    return DISubprogram::getDistinct(std::forward<Ts>(Args)...);
> +  return DISubprogram::get(std::forward<Ts>(Args)...);
> +}
> +
>  DISubprogram *DIBuilder::createFunction(DIScope *Context, StringRef Name,
>                                          StringRef LinkageName, DIFile *File,
>                                          unsigned LineNo, DISubroutineType *Ty,
> @@ -684,12 +691,13 @@ DISubprogram *DIBuilder::createFunction(
>                                          MDNode *TParams, MDNode *Decl) {
>    assert(Ty->getTag() == dwarf::DW_TAG_subroutine_type &&
>           "function types should be subroutines");
> -  auto *Node = DISubprogram::get(
> -      VMContext, DIScopeRef::get(getNonCompileUnitScope(Context)), Name,
> -      LinkageName, File, LineNo, Ty, isLocalToUnit, isDefinition, ScopeLine,
> -      nullptr, 0, 0, Flags, isOptimized, Fn, cast_or_null<MDTuple>(TParams),
> -      cast_or_null<DISubprogram>(Decl),
> -      MDTuple::getTemporary(VMContext, None).release());
> +  auto *Node = getSubprogram(/* IsDistinct = */ isDefinition, VMContext,
> +                             DIScopeRef::get(getNonCompileUnitScope(Context)),
> +                             Name, LinkageName, File, LineNo, Ty, isLocalToUnit,
> +                             isDefinition, ScopeLine, nullptr, 0, 0, Flags,
> +                             isOptimized, Fn, cast_or_null<MDTuple>(TParams),
> +                             cast_or_null<DISubprogram>(Decl),
> +                             MDTuple::getTemporary(VMContext, None).release());
> 
>    if (isDefinition)
>      AllSubprograms.push_back(Node);
> @@ -723,11 +731,12 @@ DIBuilder::createMethod(DIScope *Context
>           "Methods should have both a Context and a context that isn't "
>           "the compile unit.");
>    // FIXME: Do we want to use different scope/lines?
> -  auto *SP = DISubprogram::get(
> -      VMContext, DIScopeRef::get(cast<DIScope>(Context)), Name, LinkageName, F,
> -      LineNo, Ty, isLocalToUnit, isDefinition, LineNo,
> -      DITypeRef::get(VTableHolder), VK, VIndex, Flags, isOptimized, Fn,
> -      cast_or_null<MDTuple>(TParam), nullptr, nullptr);
> +  auto *SP = getSubprogram(/* IsDistinct = */ isDefinition, VMContext,
> +                           DIScopeRef::get(cast<DIScope>(Context)), Name,
> +                           LinkageName, F, LineNo, Ty, isLocalToUnit,
> +                           isDefinition, LineNo, DITypeRef::get(VTableHolder),
> +                           VK, VIndex, Flags, isOptimized, Fn,
> +                           cast_or_null<MDTuple>(TParam), nullptr, nullptr);
> 
>    if (isDefinition)
>      AllSubprograms.push_back(SP);
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 