Page MenuHomePhabricator

[DebugInfo] Append extended dereferencing mechanism to variables' DIExpression for targets that support more than one address space
ClosedPublic

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

Diff Detail

Repository
rL LLVM

Event Timeline

kzhuravl created this revision.Feb 7 2017, 12:22 PM
arsenm added inline comments.Feb 7 2017, 12:29 PM
lib/CodeGen/CGDebugInfo.cpp
3351 ↗(On Diff #87498)

Is this hook necessary? Can you just not append if the returned address space is the default, or do we want to be explicit even with private address space?

dblaikie added inline comments.Feb 7 2017, 12:33 PM
lib/Basic/Targets.cpp
2275 ↗(On Diff #87498)

Is this necessary? Or could it be done only when the address space is non-zero (so the getTargetAddressspace would be called unconditionally, if the result is non-zero add the addr space xderef thing)?

aprantl added inline comments.Feb 7 2017, 12:45 PM
lib/CodeGen/CGDebugInfo.cpp
3187 ↗(On Diff #87498)

MutableArrayRef?

test/CodeGenOpenCL/debugger-amdgpu-variable-locations.cl
49 ↗(On Diff #87498)

Does this work in a noasserts build where most names are stripped from IR?

tony-tye added inline comments.Feb 7 2017, 2:24 PM
lib/Basic/Targets.cpp
2275 ↗(On Diff #87498)

I had the same thought. I think the question is what assumption can be made about the consumer of the DWARF. I do not think DWARF requires that an omitted address space should be treated as 0, so if a target chooses not to do so then omitting the address space is not necessarily the same as generating a 0 address space. At least always generating them is safe!

Another issue is what the consumer of the DWARF requires. For AMDGPU address space 0 is private since alloca allocates space in the per-thread address space. To support address spaces the debugger needs a core address that can access any of the address spaces, which would be the generic address space. The XDREF is telling the debugger to do the affect of an address space cast, which is needed to map from the private address space to the generic address space. So omitting it would prevent the target specific mapping to happen.

Another observation is that the target specific address space numbering in LLVM need not necessarily be the address class/address space numbering in DWARF. So should there be a mapping function? If such a function existed, then the target could define DWARF address space 0 to be generic (and so need no conversion) and then address class and XDREF could be omitted for DWARF address class/space 0 (which for AMDGPU would not map to LLVM address space 0).

arsenm edited edge metadata.Feb 7 2017, 2:38 PM

It would also be good if we could leave open the possibility of decoupling LLVM address space 0 from OpenCL private address space. I would like to be able to someday be able to change that

frej added a subscriber: frej.Feb 13 2017, 4:24 AM
frej added inline comments.Feb 14 2017, 5:24 AM
lib/Basic/Targets.cpp
2275 ↗(On Diff #87498)

Missing override.

frej added inline comments.Feb 15 2017, 4:43 AM
lib/Basic/Targets.cpp
2275 ↗(On Diff #87498)

Another observation is that the target specific address space numbering in LLVM need not necessarily be the address class/address space numbering in DWARF.

For the out-of-tree target I'm involved with, this is the case.

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

Address review feedback

lib/Basic/Targets.cpp
2275 ↗(On Diff #87498)

Added a function for mapping LLVM AS -> DWARF AS.

lib/CodeGen/CGDebugInfo.cpp
3187 ↗(On Diff #87498)

I have currently changed it to SmallVectorImpl. I am not sure if MutableArrayRef should be used here?

What if we decide to add more expressions before appending address space xderef?

3351 ↗(On Diff #87498)

We want to be explicit about private and lds.

test/CodeGenOpenCL/debugger-amdgpu-variable-locations.cl
49 ↗(On Diff #87498)

Fixed. Thanks.

This revision is now accepted and ready to land.Feb 16 2017, 7:45 PM
aprantl added inline comments.Feb 17 2017, 9:31 AM
lib/CodeGen/CGDebugInfo.cpp
3187 ↗(On Diff #87498)

SmallVectorImpl is fine.

kzhuravl updated this revision to Diff 88976.Feb 17 2017, 3:36 PM

No changes, just a rebase

This revision was automatically updated to reflect the committed changes.