This is an archive of the discontinued LLVM Phabricator instance.

PR27284 - Reverse the ownership between DICompileUnit and DISubprogram
ClosedPublic

Authored by aprantl on Apr 12 2016, 2:56 PM.

Details

Summary

Currently each Function points to a DISubprogram and DISubprogram has a scope field. For member functions the scope is a DICompositeType. DIScopes never point to the DICompileUnit to facilitate type uniquing.

Distinct DISubprograms (with isDefinition: true) are not part of the type hierarchy and cannot be uniqued.This patch removes the subprograms list from DICompileUnit and instead adds a pointer to the owning compile unit to distinct DISubprograms. This enables ThinLTO to lazy-load DISubprograms and their transitively referenced debug info.

Motivation

Materializing DISubprograms is currently the most expensive operation when doing a ThinLTO build of clang.

We want the DISubprogram to be stored in a separate Bitcode block (or the same block as the function body) so we can avoid having to expensively deserialize all DISubprograms together with the global metadata. If a function has been inlined into another subprogram we need to store a reference the block containing the inlined subprogram.

Notes

Attached to https://llvm.org/bugs/show_bug.cgi?id=27284 is a python script that updates LLVM IR testcases to the new format.

This patch does not include the upgraded clang testcases.

Diff Detail

Event Timeline

aprantl updated this revision to Diff 53468.Apr 12 2016, 2:56 PM
aprantl retitled this revision from to PR27284 - Reverse the ownership between DICompileUnit and DISubprogram.
aprantl updated this object.
aprantl added reviewers: dexonsmith, dblaikie, echristo.
aprantl set the repository for this revision to rL LLVM.
aprantl added subscribers: mehdi_amini, tejohnson, pcc and 2 others.
aprantl updated this revision to Diff 53484.Apr 12 2016, 3:56 PM
aprantl edited edge metadata.
aprantl removed rL LLVM as the repository for this revision.
  • rebased on 266137
  • fix DIBuilder to not set the unit field for declarations
dblaikie accepted this revision.Apr 13 2016, 10:53 AM
dblaikie edited edge metadata.

Generally looks good - bunch of cleanup sort of stuff & a few casual questions, none of which I think would block this being committed.

lib/Bitcode/Reader/BitcodeReader.cpp
1983–1985

Indentation (looks like the for SPs loop is indented at the same level as the if CU_SP.second condition - though it appears as though it's semantically nested inside it)

2255

Can't remember if this braced init works in MSVC, you might end up needing to use make_pair

2266

You could drop the conditional operator here, if you like - booleans convert to 1 and 0. Up to you if you think that's sufficiently/more readable, etc. (could even leave the variable as type bool, too). I could go either way.

lib/Bitcode/Writer/BitcodeWriter.cpp
1101

Not worth changing the record layout to have one fewer records?

lib/Transforms/Instrumentation/GCOVProfiling.cpp
460–462

Maybe pull this out as a separate patch (& the change to the parameter type from pointer to reference) (& the other range-for-ification later in this loop, etc)

582–584

Separate patch?

603–605

Separate patch?

lib/Transforms/Utils/CloneFunction.cpp
181

Maybe make this an explicit "FIXME"? That extra cloning work seems significant, no? (especially if it means cloning all the types in the CU, etc?)

This revision is now accepted and ready to land.Apr 13 2016, 10:53 AM
davide edited edge metadata.Apr 14 2016, 9:25 AM
davide added a subscriber: davide.
aprantl closed this revision.Apr 15 2016, 9:05 AM

Thanks everyone!
Committed as r266445+r266446.

Hi all,

Just wanted to share with everyone that I profiled importing IR with ThinLTO, and this change makes linkInModule() ~10x faster!

Great work Adrian :)