Details
Diff Detail
Event Timeline
lib/Bitcode/Reader/MetadataLoader.cpp | ||
---|---|---|
1122–1126 | Where is this limit from? The enforced address space limit in other places is 24 bits |
lib/Bitcode/Reader/MetadataLoader.cpp | ||
---|---|---|
1122–1126 | I was not sure about that. Will fix. Thanks. |
I think this *generally* looks fine, there should also be a binary bitcode upgrade testcase that verifies the code form MetadataLoader.cpp:1080
lib/CodeGen/AsmPrinter/DwarfUnit.cpp | ||
---|---|---|
847 | 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. |
Address review feedback:
- Switch address space member in DIDerivedType class from unsigned to Optional<unsigned>
- Remove target hook
- Add bitcode upgrade test
Done, thanks.
lib/CodeGen/AsmPrinter/DwarfUnit.cpp | ||
---|---|---|
847 | Hook is not needed since we switched to Optional. |
lib/Bitcode/Reader/MetadataLoader.cpp | ||
---|---|---|
1117 | What's the magical ~0U value about? | |
lib/CodeGen/AsmPrinter/DwarfUnit.cpp | ||
847–848 | 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 | maybe reuse a variable here (& probably the op* on Optional) if (const auto &AS = ...) { ..., *AS, ... |
lib/Bitcode/Reader/MetadataLoader.cpp | ||
---|---|---|
1117 | 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 | 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+. |
lib/Bitcode/Reader/MetadataLoader.cpp | ||
---|---|---|
1117 | I'm pretty sure we prefer bitcode records from the same LLVM revision to always have the same number of elements. |
test/Bitcode/pointer-address-space.ll | ||
---|---|---|
5 | 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? |
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)
include/llvm/IR/DebugInfoMetadata.h | ||
---|---|---|
715 | 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? |
Hi Adrian and David, can you take a final look over the patches when you get a chance?
Thanks,
Konstantin
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?