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

[Konstantin Zhuravlyov via Phabricator via llvm-commits]

kzhuravl added inline comments.


================
Comment at: lib/Basic/Targets.cpp:2275
+  /// false otherwise.
+  bool targetSupportsMultipleAddressSpaces() const {
+    return true;
----------------
frej wrote:
> frej wrote:
> > tony-tye wrote:
> > > dblaikie wrote:
> > > > 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)?
> > > 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).
> > Missing `override`.
> > 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.
> 
> 
Added a function for mapping LLVM AS -> DWARF AS.


================
Comment at: lib/CodeGen/CGDebugInfo.cpp:3187
+    unsigned AddressSpace,
+    SmallVector<int64_t, 9> &Expr) const {
+  Expr.push_back(llvm::dwarf::DW_OP_constu);
----------------
aprantl wrote:
> MutableArrayRef?
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? 


================
Comment at: lib/CodeGen/CGDebugInfo.cpp:3351
+  // Append extended dereferencing mechanism for VD's address space.
+  if (CGM.getTarget().targetSupportsMultipleAddressSpaces()) {
+    unsigned AddressSpace =
----------------
arsenm wrote:
> 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?
We want to be explicit about private and lds.


================
Comment at: test/CodeGenOpenCL/debugger-amdgpu-variable-locations.cl:49
+// CHECK-DAG: call void @llvm.dbg.declare(metadata i32 addrspace(2)** %KernelArg1.addr, metadata !{{[0-9]+}}, metadata ![[ADDRSPACE0]])
+// CHECK-DAG: call void @llvm.dbg.declare(metadata i32 addrspace(3)** %KernelArg2.addr, metadata !{{[0-9]+}}, metadata ![[ADDRSPACE0]])
+kernel void kernel1(global int *KernelArg0,
----------------
aprantl wrote:
> Does this work in a noasserts build where most names are stripped from IR?
Fixed. Thanks.


https://reviews.llvm.org/D29673