Page MenuHomePhabricator

[DebugInfo] Add address space when creating DIDerivedTypes
ClosedPublic

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

Diff Detail

Repository
rL LLVM

Event Timeline

kzhuravl created this revision.Feb 7 2017, 12:18 PM
arsenm added inline comments.Feb 7 2017, 12:22 PM
lib/Basic/Targets.cpp
2270 ↗(On Diff #87496)

Should this be constant in some contexts?

dblaikie added inline comments.Feb 7 2017, 12:36 PM
test/CodeGenOpenCL/debugger-pointer-address-space.cl
1 ↗(On Diff #87496)

Seems like a lot of test coverage for 4 changes. Is it all needed? I'd expect roughly 4 tests for these 4 changes. One for each that demonstrates the addr space is plumbed through. Probably no need to test that all addr spaces work in each context - because this code is pretty/entirely orthogonal to the particular address space.

arsenm added inline comments.Feb 7 2017, 12:39 PM
test/CodeGenOpenCL/debugger-pointer-address-space.cl
1 ↗(On Diff #87496)

I think it's better to explicitly test all address spaces the target cares about

frej added a subscriber: frej.Feb 13 2017, 4:23 AM
frej added inline comments.Feb 14 2017, 5:25 AM
lib/Basic/Targets.cpp
2269 ↗(On Diff #87496)

Missing override

kzhuravl updated this revision to Diff 88666.Feb 16 2017, 12:46 AM
kzhuravl marked an inline comment as done.

Address review feedback:

  • Add target hook for asking about LLVM AS -> DWARF AS
  • Use Optional<unsigned> for DWARF AS
kzhuravl added inline comments.Feb 16 2017, 12:47 AM
lib/Basic/Targets.cpp
2270 ↗(On Diff #87496)

I can't think of any at this moment.

dblaikie added inline comments.Feb 16 2017, 9:46 AM
include/clang/Basic/TargetInfo.h
1043 ↗(On Diff #88666)

Is this a reasonable default implementation? Should there be a default or should any implementation (mabe only those with non-zero address spaces?) be expected to provide a mapping? (not sure how this looks for other things in TargetInfo)

tony-tye added inline comments.Feb 16 2017, 7:40 PM
include/clang/Basic/TargetInfo.h
1040–1041 ↗(On Diff #88666)

Perhaps give a more detailed description:

"\return If a target requires an address within a target specific address space \p AddressSpace to be converted in order to be used, then return the corresponding target specific DWARF address space.
\return Otherwise return None and no conversion will be emitted in the DWARF."

lib/Basic/Targets.cpp
2259–2260 ↗(On Diff #88666)

Same comment as above.

kzhuravl updated this revision to Diff 88950.Feb 17 2017, 1:44 PM
kzhuravl marked 2 inline comments as done.

Address review feedback

include/clang/Basic/TargetInfo.h
1043 ↗(On Diff #88666)

I think the proposed hook is flexible (targets can map address spaces however they like) and returning None is a reasonable default (dwarf spec does not say that DW_AT_address_class is required - returning None here, will cause AsmPrinter not to generate DW_AT_address_class).

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

AddressSpace -> DWARFAddressSpace.

tony-tye added inline comments.Feb 21 2017, 2:27 PM
lib/CodeGen/CGDebugInfo.cpp
1612 ↗(On Diff #89277)

Should this be using the Constant address space since presumably a vtable will be in constant memory and not change during the execution of the program? This allows better code generation as constant data can be loaded by scalar operations on GPU hardware.

1657 ↗(On Diff #89277)

Same comment as above.

kzhuravl updated this revision to Diff 89410.Feb 22 2017, 12:42 PM
kzhuravl marked 2 inline comments as done.

Address review feedback.

This revision is now accepted and ready to land.Feb 22 2017, 12:46 PM
frej added inline comments.Feb 23 2017, 1:29 AM
lib/Basic/Targets.cpp
2262 ↗(On Diff #89410)

This one is missing an 'override'

kzhuravl updated this revision to Diff 89538.Feb 23 2017, 11:30 AM
kzhuravl marked an inline comment as done.

Add missing override keyword.

This revision was automatically updated to reflect the committed changes.