Page MenuHomePhabricator

[DebugInfo] Emit address space with DW_AT_address_class attribute for pointer and reference types
ClosedPublic

Authored by kzhuravl on Feb 7 2017, 12:16 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

kzhuravl created this revision.Feb 7 2017, 12:16 PM
arsenm added inline comments.Feb 7 2017, 12:31 PM
lib/Bitcode/Reader/MetadataLoader.cpp
1078–1082 ↗(On Diff #87495)

Where is this limit from? The enforced address space limit in other places is 24 bits

kzhuravl added inline comments.Feb 7 2017, 12:32 PM
lib/Bitcode/Reader/MetadataLoader.cpp
1078–1082 ↗(On Diff #87495)

I was not sure about that. Will fix. Thanks.

aprantl edited edge metadata.Feb 7 2017, 12:42 PM

I think this *generally* looks fine, there should also be a binary bitcode upgrade testcase that verifies the code form MetadataLoader.cpp:1080

dblaikie added inline comments.Feb 7 2017, 12:44 PM
lib/CodeGen/AsmPrinter/DwarfUnit.cpp
847 ↗(On Diff #87495)

Similar feedback to one of the other patches - whether or not this hook (targetSupportsMultipleAddressSpaces) is needed or if the "not default address space" (or not zero adderss space) would suffice.

frej added a subscriber: frej.Feb 13 2017, 4:23 AM
kzhuravl updated this revision to Diff 88665.Feb 16 2017, 12:43 AM
kzhuravl marked 2 inline comments as done.

Address review feedback:

  • Switch address space member in DIDerivedType class from unsigned to Optional<unsigned>
  • Remove target hook
  • Add bitcode upgrade test

I think this *generally* looks fine, there should also be a binary bitcode upgrade testcase that verifies the code form MetadataLoader.cpp:1080

Done, thanks.

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
847 ↗(On Diff #87495)

Hook is not needed since we switched to Optional.

dblaikie added inline comments.Feb 16 2017, 9:42 AM
lib/Bitcode/Reader/MetadataLoader.cpp
1117 ↗(On Diff #88665)

What's the magical ~0U value about?

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
847–848 ↗(On Diff #88665)

why only on pointer and reference types? Should we just do it on all types and don't put an address space on osmething in the IR if it's not desired to be in the DWARF?

lib/IR/AsmWriter.cpp
1617–1618 ↗(On Diff #88665)

maybe reuse a variable here (& probably the op* on Optional)

if (const auto &AS = ...) {
  ..., *AS, ...
kzhuravl added inline comments.Feb 16 2017, 1:53 PM
lib/Bitcode/Reader/MetadataLoader.cpp
1117 ↗(On Diff #88665)

I used it as a sentinel value to represent "None" optional dwarf address space in the bitcode (we always generate and read address space record, if it is ~0U, then we pass None when creating DIDerivedType).

Would it be better to not write address space record if it is None?

We can also use UINT24_MAX + 1 (we would need to define UINT24_MAX). or ADDRESS_SPACE_MAX?

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
847–848 ↗(On Diff #88665)

According to dwarf specs [v2-v5], DW_AT_address_class only applies to pointer or reference types, subroutines or subroutine types. I just checked, and DW_AT_address_class is not present in dwarf spec v1. So we should only generate DW_AT_address_class for v2+.

aprantl added inline comments.Feb 16 2017, 2:10 PM
lib/Bitcode/Reader/MetadataLoader.cpp
1117 ↗(On Diff #88665)

I'm pretty sure we prefer bitcode records from the same LLVM revision to always have the same number of elements.
Typically we use 0 as the default value and omit it in the textual IR.
What would you think about encoding the address space as n+1, with 0 being the default=None value?

aprantl added inline comments.Feb 17 2017, 9:28 AM
test/Bitcode/pointer-address-space.ll
5 ↗(On Diff #88665)

The point of the bitcode test is to ensure that the bitcode upgrade from an *older* version (without the address space field, i.e., today's LLVM trunk) is handled correctly. The IR assembler round-trip tests are already testing that the *current* bitcode is handled correctly.

The fact that this test is checking for addressSpace: 2 makes it look like the .bc file was generated with this patch and not with current LLVM trunk?

kzhuravl updated this revision to Diff 88948.Feb 17 2017, 1:43 PM
kzhuravl marked 9 inline comments as done.

Address review feedback:

  • Encode the address space in bitcode as n+1, with 0 being the default=None value
  • Updated verifier to check that address space is only present in DW_AT_pointer_type and DW_AT_reference_type (+ relevant tests)
  • Added test to check that DW_AT_address_class is not emitted in DWARF v1
  • Other minor feedback (marked inline comments as done)
kzhuravl added inline comments.Feb 18 2017, 11:29 AM
include/llvm/IR/DebugInfoMetadata.h
715 ↗(On Diff #88948)

Since this is a mapped address space and not LLVM address space (in our case it is Target-specific DWARF Address Space), the name should reflect that.

We were thinking of prefixing it with "DI", but not sure what the policy is here. Seems like types are prefixed with "DI".

Would there be any objections if we rename it to DIAddressSpace?

kzhuravl updated this revision to Diff 89276.Feb 21 2017, 2:05 PM

AddressSpace -> DWARFAddressSpace.

This revision is now accepted and ready to land.Feb 21 2017, 2:27 PM

Hi Adrian and David, can you take a final look over the patches when you get a chance?

Thanks,
Konstantin

scchan accepted this revision.Mar 1 2017, 7:12 AM

LGTM

Two tiny nitpicks, but apart from that I'm happy.

lib/AsmParser/LLParser.cpp
3930 ↗(On Diff #89409)

I think the = None is redundant.

lib/Bitcode/Reader/MetadataLoader.cpp
1118 ↗(On Diff #89409)

I think the = None is redundant

This revision was automatically updated to reflect the committed changes.
kzhuravl marked 2 inline comments as done.