Page MenuHomePhabricator

[WIP] DebugInfo: New metadata representation for global variables.
ClosedPublic

Authored by pcc on May 10 2016, 6:34 PM.

Details

Summary

This patch reverses the edge from DIGlobalVariable to GlobalVariable. In
order to make this work I also needed to implement the following extensions
for global attached metadata:

  • I implemented a capability for globals to hold multiple metadata nodes with the same kind.
  • I added a byte offset field to the global attached metadata list entries. The byte offset can be used to describe the position in the global that the metadata pertains to. Passes such as merge globals can copy metadata from one global to another, adjusting the offset if necessary. This field is intended to be generically useful, for example I plan to use it to hold the offsets currently used by "bitset" metadata in the vtable opt and CFI passes.

This isn't fully ready yet, but I wanted to gauge support for the new
representation before implementing the remaining pieces.

TODO:

  • Add bitcode read/write and upgrade support for the new constructs
  • Use a DIExpression to represent constants
  • Update ASan to copy metadata in the same way as I updated merge globals
  • Update clang

Depends on D20074

Diff Detail

Event Timeline

pcc updated this revision to Diff 56848.May 10 2016, 6:34 PM
pcc retitled this revision from to [WIP] DebugInfo: New metadata representation for global variables..
pcc added reviewers: dexonsmith, dblaikie, aprantl.
pcc updated this object.
pcc added a subscriber: llvm-commits.
pcc added a comment.May 10 2016, 6:38 PM

I have attached the Go program that I used to upgrade most of the test cases.

tstellarAMD added a subscriber: kzhuravl.
aprantl edited edge metadata.May 11 2016, 10:07 AM

Instead of baking a byte-offset into the IR, I was more thinking of using the DIExpression to describe the offset:

For example, to describe a global at offset 4, we would use a DIExpression(DW_OP_constu 4, DW_OP_plus) to adjust the offset.

test/CodeGen/ARM/2011-08-02-MergedGlobalDbg.ll
1

Just a general remark: This should really be split into one test that runs opt -arm-global-merge, and one that runs llc and checks the DWARF output.

aprantl edited edge metadata.May 11 2016, 10:07 AM
aprantl added a subscriber: probinson.

If we agree that using the DIExpression is how we want to proceed, it might be easier to stage this by introducing a DIExpression to DIGlobalVariable first in a separate commit.

pcc added a comment.May 11 2016, 10:19 AM

Instead of baking a byte-offset into the IR, I was more thinking of using the DIExpression to describe the offset:

I suppose we could do that, but I reckon that would make things more complicated:

  • each constant would need two DIExpressions, one for the case where a global exists and an address can be derived from the attached GlobalVariable's address, and one for the case where the global has been optimized away (where we'd use DW_OP_stack_value to describe the constant)
  • it would add complexity to the copyMetadata function, as it would need to be able to manipulate and rewrite DIExpressions
  • I also plan to use the offset in other kinds of metadata attached to globals, such as the type metadata for CFI and vtable opt. If we use a separate representation for each kind of metadata, copyMetadata would need to know how to adjust the offset in each kind of metadata
In D20147#427453, @pcc wrote:

Instead of baking a byte-offset into the IR, I was more thinking of using the DIExpression to describe the offset:

I suppose we could do that, but I reckon that would make things more complicated:

  • each constant would need two DIExpressions, one for the case where a global exists and an address can be derived from the attached GlobalVariable's address, and one for the case where the global has been optimized away (where we'd use DW_OP_stack_value to describe the constant)

Not sure if I follow. While the constant still exists as a global in the IR, the DIExpression would be empty.
Whenever a constant is deleted from IR, we invoke a utility that

  • adds the DIGlobalVariable to the DICompileUnit to retain it
  • translates the constexpr into a DIExpression

This is assuming that after GlobalMerge happened no merged globals will be removed.

  • it would add complexity to the copyMetadata function, as it would need to be able to manipulate and rewrite DIExpressions

Can you give a quick example for this?

  • I also plan to use the offset in other kinds of metadata attached to globals, such as the type metadata for CFI and vtable opt. If we use a separate representation for each kind of metadata, copyMetadata would need to know how to adjust the offset in each kind of metadata
pcc added a comment.May 11 2016, 11:01 AM
In D20147#427453, @pcc wrote:

Instead of baking a byte-offset into the IR, I was more thinking of using the DIExpression to describe the offset:

I suppose we could do that, but I reckon that would make things more complicated:

  • each constant would need two DIExpressions, one for the case where a global exists and an address can be derived from the attached GlobalVariable's address, and one for the case where the global has been optimized away (where we'd use DW_OP_stack_value to describe the constant)

Not sure if I follow. While the constant still exists as a global in the IR, the DIExpression would be empty.
Whenever a constant is deleted from IR, we invoke a utility that

  • adds the DIGlobalVariable to the DICompileUnit to retain it

I'm not changing the relationship between DICompileUnit and DIGlobalVariable in this patch. This means that if we change the DIExpression, we'd most likely need to find the DIGlobalVariable in the DICompileUnit's global list and rewrite it. But I suppose that's fine if it's temporary.

  • translates the constexpr into a DIExpression This is assuming that after GlobalMerge happened no merged globals will be removed.

Okay. So if I follow you, the interpretation of the DIExpression will change depending on whether the DIGlobalVariable was attached to a GlobalVariable. If it is attached, we'd start with the address of the GlobalVariable on the stack, and if it isn't attached we'd start with an empty stack. Is that what you had in mind?

  • it would add complexity to the copyMetadata function, as it would need to be able to manipulate and rewrite DIExpressions

Can you give a quick example for this?

Well, okay, I suppose that if we're just adding an offset it wouldn't be too hard to append (DW_OP_constu, X, DW_OP_plus) to the DIExpression. But we'd (yes, temporarily) need to rewrite the DICompileUnit's global list, and each new kind of metadata node that we attach to globals would require its own handling.

In D20147#427547, @pcc wrote:
In D20147#427453, @pcc wrote:

Instead of baking a byte-offset into the IR, I was more thinking of using the DIExpression to describe the offset:

I suppose we could do that, but I reckon that would make things more complicated:

  • each constant would need two DIExpressions, one for the case where a global exists and an address can be derived from the attached GlobalVariable's address, and one for the case where the global has been optimized away (where we'd use DW_OP_stack_value to describe the constant)

Not sure if I follow. While the constant still exists as a global in the IR, the DIExpression would be empty.
Whenever a constant is deleted from IR, we invoke a utility that

  • adds the DIGlobalVariable to the DICompileUnit to retain it

I'm not changing the relationship between DICompileUnit and DIGlobalVariable in this patch. This means that if we change the DIExpression, we'd most likely need to find the DIGlobalVariable in the DICompileUnit's global list and rewrite it. But I suppose that's fine if it's temporary.

Agreed, that sounds not so great. Would it be possible to change the relationship before landing this patch?

  • translates the constexpr into a DIExpression This is assuming that after GlobalMerge happened no merged globals will be removed.

Okay. So if I follow you, the interpretation of the DIExpression will change depending on whether the DIGlobalVariable was attached to a GlobalVariable. If it is attached, we'd start with the address of the GlobalVariable on the stack, and if it isn't attached we'd start with an empty stack. Is that what you had in mind?

The DIExpressions in LLVM IR have an implicit first element that is the "base" location of the entity that is being described. The DWARF backend can make the following decision:

  • if the DIGlobalVariable is found via a !dbg attachment on a global, the address of the global is emitted as the first element of the dwarf expression, followed by the DIExpression,
  • if the DIExpression describes a constant (i.e., it ends with DW_OP_stack_value, DW_OP_implicit_value, DW_OP_implicit_pointer, ...) the DIExpression is emitted as is.
  • it would add complexity to the copyMetadata function, as it would need to be able to manipulate and rewrite DIExpressions

Can you give a quick example for this?

Well, okay, I suppose that if we're just adding an offset it wouldn't be too hard to append (DW_OP_constu, X, DW_OP_plus) to the DIExpression. But we'd (yes, temporarily) need to rewrite the DICompileUnit's global list, and each new kind of metadata node that we attach to globals would require its own handling.

Nitpick: I think it would need to prepend the DW_OP_constu, X, DW_OP_plus.

pcc updated this revision to Diff 57737.May 18 2016, 10:41 PM

Refresh and split out dependencies

pcc added a comment.May 18 2016, 10:45 PM

Okay, I've updated this per Adrian's suggestions, split out some of the dependencies (D20412, D20413, D20414) and created a clang patch (D20415). This should be ready for review now.

pcc updated this revision to Diff 57834.May 19 2016, 11:52 AM
  • Only emit the expr if we have a global and the expr form is not specifically recognized

Thanks for putting so much effort into this, this is starting to look really great!

include/llvm/IR/DIBuilder.h
503

If we haven't already, we will need to add explicit support for DW_OP_stack_value to DwarfExpression to ensure that we don't emit when generating DWARF 2. (And yes, without it the expression will be ambiguous).

include/llvm/IR/DebugInfoMetadata.h
1827

... or is the (constant) variable.

1830

Oh yes :-)

1867

All the \brief's are redundant unless you want to include more than the first sentence in the brief description. Makes the code a bit more readable IMHO.
(I know this is just copied, but we might as well fix it up now).

lib/Bitcode/Reader/BitcodeReader.cpp
2607

I'm not sure if Expr is guaranteed to be materialized at this point. You may need to store a list of DIExpressions and fix them up later.
Duncan?

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
126

Ah, great! This should probably say "DWARF 3 and earlier",

pcc updated this revision to Diff 57986.May 20 2016, 1:53 PM
pcc marked 3 inline comments as done.
  • Address review comments
lib/Bitcode/Reader/BitcodeReader.cpp
2607

My understanding is that lazy materialization is only used for functions, which is why we need the fixup code for upgrading function metadata, but not for globals.

aprantl added inline comments.May 31 2016, 12:05 PM
include/llvm/IR/DIBuilder.h
444

Maybe this should default to nullptr?

include/llvm/IR/GlobalVariable.h
32

Why Module?

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
179–180

Can you remind me why this is no longer necessary?

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
470

Nice! This should probably be a separate NFC commit.

491
if (const GlobalVariable *Global = GVMap.lookup(GV))
  CU.getOrCreateGlobalVariableDIE(GV, Global);

?

lib/CodeGen/GlobalMerge.cpp
460

Maybe add a short comment about what's happening here?

lib/IR/DebugInfoMetadata.cpp
566

Technically correct, but DW_OP_piece must be even farther at the end ;-)

In the DWARF 5 draft Figure D.61, there is an expression example that uses both to synthesize a struct from the constants {1, 2}:

DW_OP_lit1 DW_OP_stack_value DW_OP_piece(4)
DW_OP_lit2 DW_OP_stack_value DW_OP_piece(4)
pcc updated this revision to Diff 59168.May 31 2016, 8:12 PM
pcc marked 4 inline comments as done.
  • Refresh
  • Address review comments
  • Simplify: do not use a separate loop to create CU DIEs.
include/llvm/IR/GlobalVariable.h
32

Just a drive-by cleanup, I was re-ordering these declarations alphabetically.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
179–180

Both of these cases are now represented using expressions: the first using DW_OP_stack_value, and the second using DW_OP_plus to apply the adjustment.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
470

r271360

491

Yes, almost, but we still need to create DIEs for unattached globals here.

lib/IR/DebugInfoMetadata.cpp
566

Okay, adjusted to say what we currently support.

aprantl added inline comments.Jun 1 2016, 9:16 AM
lib/IR/DebugInfoMetadata.cpp
566

That's probably good enough for this patch.

pcc updated this revision to Diff 59259.Jun 1 2016, 11:39 AM
  • Simplify: Remove unnecessary temporary variable
pcc added a comment.Jun 2 2016, 1:01 PM

We currently aren't correctly deserializing global variable metadata from lazy loaded modules, so should probably hold off until that's fixed.

dexonsmith edited edge metadata.Jun 2 2016, 3:22 PM
dexonsmith added a subscriber: dexonsmith.

One fix for that would be to lazy-load global variables/aliases, and load the metadata block when the first global variable is accessed.

pcc added a comment.Jun 2 2016, 3:32 PM

The fix I'm working on is to move the GLOBALVAR_ATTACHMENT record to the METADATA block. I'll send it out in a bit.

I wonder if that's the right long term solution?

Lazy-loading GlobalVariables could eliminate the global constants block, and we could move GlobalVariable-related metadata out of the global metadata block as well.

I think lazy-loading GlobalVariables could result in important speedups for -flto=thin, and solves this problem as a side effect.

It would also speedup llvm-ar, and other tools that use lazy-loading.

pcc added a comment.Jun 2 2016, 4:29 PM

While it's possible that that would result in a performance improvement, it's unclear that that would be the case, and I don't think we should make such a change without evidence. For ThinLTO and llvm-ar a more effective performance improvement would be to pre-compute the symbol table (see pr27551), which would avoid the need to read most of the bitcode file. There's also the possibility that lazy loading GlobalVariables would slow things down, as the benefit of lazy loading globals would generally be less than for functions.

I agree with you on llvm-ar.
For ThinLTO it is totally different and the symbol table does not help: when you want to import a function, you need to lazy-load the module and materialize the functions you want to import.
This is where we want as much laziness with other globals and more importantly metadata in general.

pcc updated this revision to Diff 59819.Jun 6 2016, 7:41 PM
pcc edited edge metadata.
  • Refresh

While going through my review queue I was wondering what is the state of this?

pcc added a comment.Sep 9 2016, 3:11 PM

My recollection is that the lazy loading issues discussed are not a blocker, as they do not represent a perf regression (DIGlobalVariable metadata is already being loaded eagerly). Changing the bitcode representation for global variables to allow for lazy loading can be done separately.

If there are no other issues this patch most likely just needs a sign off (and a refresh given its age).

In D20147#539056, @pcc wrote:

My recollection is that the lazy loading issues discussed are not a blocker, as they do not represent a perf regression (DIGlobalVariable metadata is already being loaded eagerly). Changing the bitcode representation for global variables to allow for lazy loading can be done separately.

If there are no other issues this patch most likely just needs a sign off (and a refresh given its age).

I may misunderstand what this will change, it seems to me that this is a regression in lazy loading. Currently lazy loading bitcode does *not* load global metadata, while after this change it will load all what is reachable from the metadata attached to a global.
It would matter when the linker lazy load to perform symbol resolution for instance, which is a blocker I think.

On the other side, if something like D23132 lands, that would probably unblock this as we would have another path than lazy-loading bitcode.

pcc added a comment.Sep 9 2016, 3:53 PM

Currently lazy loading bitcode does *not* load global metadata, while after this change it will load all what is reachable from the metadata attached to a global.

I don't think that's accurate. The metadata will continue to be stored in the global metadata block, and if the client requests lazy loading of metadata will only be loaded during materializeMetadata (alongside other metadata in the global metadata block), except that now it will be attached to the global using a METADATA_GLOBAL_DECL_ATTACHMENT record.

In D20147#539124, @pcc wrote:

Currently lazy loading bitcode does *not* load global metadata, while after this change it will load all what is reachable from the metadata attached to a global.

I don't think that's accurate. The metadata will continue to be stored in the global metadata block, and if the client requests lazy loading of metadata will only be loaded during materializeMetadata (alongside other metadata in the global metadata block), except that now it will be attached to the global using a METADATA_GLOBAL_DECL_ATTACHMENT record.

OK, if so, then forget anything I wrote above :)

aprantl accepted this revision.Sep 12 2016, 10:03 AM
aprantl edited edge metadata.

Thanks, from my end this looks great!

If you happen to have a script to upgrade the testcases, it would be great if you could file a PR for this and attach the script there and comment in the commit message. This will make it easier for people with out-of-tree testcases.

lib/Bitcode/Reader/BitcodeReader.cpp
2604

Please make sure to add a binary bitcode upgrade test case for this.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
129

Since we already have a special createConstantValueExpression() method we could also add an Optional<uint64> getConstantValueExpression() accessor?

This revision is now accepted and ready to land.Sep 12 2016, 10:03 AM
pcc added a comment.Sep 12 2016, 5:54 PM

If you happen to have a script to upgrade the testcases, it would be great if you could file a PR for this and attach the script there and comment in the commit message

Filed PR30362 with an upgrade program.

lib/Bitcode/Reader/BitcodeReader.cpp
2604

After this patch test/Bitcode/diglobalvariable-3.8.ll and test/Bitcode/dityperefs-3.8.ll will respectively be testing the constant and variable cases.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
129

Maybe if we need to query this in more than one place. For now it seems clearer to be direct here.

This revision was automatically updated to reflect the committed changes.

Thanks for all the hard work!