This is an archive of the discontinued LLVM Phabricator instance.

DI: Reverse direction of subprogram -> function edge.
ClosedPublic

Authored by pcc on Nov 2 2015, 4:27 PM.

Details

Summary

Previously, subprograms contained a metadata reference to the function they
described. Because most clients need to get or set a subprogram for a given
function rather than the other way around, this created unneeded inefficiency.

For example, many passes needed to call the function llvm::makeSubprogramMap()
to build a mapping from functions to subprograms, and the IR linker needed to
fix up function references in a way that caused quadratic complexity in the IR
linking phase of LTO.

This change reverses the direction of the edge by storing the subprogram as
function-level metadata and removing DISubprogram's function field.

Since this is an IR change, a bitcode upgrade has been provided.

Fixes PR23367.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 39002.Nov 2 2015, 4:27 PM
pcc retitled this revision from to DI: Reverse direction of subprogram -> function edge..
pcc updated this object.
pcc added a reviewer: dexonsmith.
pcc added a subscriber: llvm-commits.
krasin added a subscriber: krasin.Nov 2 2015, 4:49 PM
axw added a comment.Nov 2 2015, 5:04 PM

bindings/go changes LGTM

aprantl added a subscriber: aprantl.Nov 3 2015, 9:52 AM

I'm impressed that we apparently never needed the other direction.
This looks good from a debug info point of view.

Two questions/remarks:

  • Could you include your upgrade script so people can upgrade their out-of-tree testcases?
  • Shouldn't we drop the function field from the DISubprogram bitcode representation?

thanks for working on this!
adrian

Generally looks good to me - one or two minor comments.

lib/Bitcode/Writer/BitcodeWriter.cpp
1027 ↗(On Diff #39002)

Could we use the size of the record to determine whether we're in the old format or the new one? Rather than including an empty field?

lib/IR/DebugInfoMetadata.cpp
364 ↗(On Diff #39002)

I wonder if this function is even needed anymore... hopefully we'll just not encounter DISubprograms for a Function that aren't attached to the same Function.

pcc updated this revision to Diff 39110.Nov 3 2015, 1:16 PM
  • Remove function field from bitcode repr
  • Defer setSubprogram until materialized
pcc added a comment.Nov 3 2015, 1:17 PM

Adrian wrote:

Could you include your upgrade script so people can upgrade their out-of-tree testcases?


Attached. This is basically the same as Duncan's upgrade script attached to PR23367 with a quick hack to remove function references from subprograms.

Duncan wrote:

This call is only valid if F is a definition. Otherwise it will assert. IIRC, metadata typically gets parsed *before* function bodies, so I think this could even rarely be valid?

It seems that this check is done in the verifier rather than in setSubprogram itself. (This actually makes more sense to me because it may be more convenient for a frontend to create the subprogram before generating code for the function.)

But I've done something along the lines of what you suggested in order to ensure that unmaterialized functions are verifiable. Because we always read function prototypes before metadata, I don't think we need to handle the case where the thing that the SP refers to is not a function.

I didn't look at tests... did you have an upgrade test?

Yes, see test/Bitcode/upgrade-subprogram.ll.

lib/Bitcode/Writer/BitcodeWriter.cpp
1027 ↗(On Diff #39002)

We can; done.

lib/IR/DebugInfoMetadata.cpp
364 ↗(On Diff #39002)

I don't think it is. If I replace this function body with just the pointer comparison the test suite still passes. But this change is pretty large as it is, so I'd prefer to make that change separately.

dexonsmith edited edge metadata.Nov 5 2015, 11:52 AM
dexonsmith added a subscriber: dexonsmith.

I have a couple of minor comments below; otherwise this LGTM, as long
as you can somehow prevent the DIBuilder API changes from inserting
bugs in frontends (more below).

This revision was automatically updated to reflect the committed changes.