[HLSL] Fix debug info generation for RWBuffer types

[joaosaffran]

This PR fix the debug infor generation for RWBuffer types.

• This implements the same fix as DXC.
• Adds the HLSLAttributedResource debug info generation

Closes #118523

[llvmbot]

@llvm/pr-subscribers-hlsl
@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-clang-codegen

Author: None (joaosaffran)

Changes

This PR fix the debug infor generation for RWBuffer types.

• This implements the same fix as DXC.
• Adds the HLSLAttributedResource debug info generation

Closes #118523

---

Full diff: https://github.com/llvm/llvm-project/pull/119041.diff

3 Files Affected:

• (modified) clang/lib/CodeGen/CGDebugInfo.cpp (+12-23)
• (modified) clang/lib/CodeGen/CGDebugInfo.h (+2)
• (added) clang/test/CodeGenHLSL/debug/rwbuffer_debug_info.hlsl (+11)

diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 60f32f76109e9a..3fb840137bbaf5 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2021,28 +2021,10 @@ llvm::DISubroutineType *CGDebugInfo::getOrCreateInstanceMethodType(
   // ThisPtr may be null if the member function has an explicit 'this'
   // parameter.
   if (!ThisPtr.isNull()) {
-    const CXXRecordDecl *RD = ThisPtr->getPointeeCXXRecordDecl();
-    if (isa<ClassTemplateSpecializationDecl>(RD)) {
-      // Create pointer type directly in this case.
-      const PointerType *ThisPtrTy = cast<PointerType>(ThisPtr);
-      uint64_t Size = CGM.getContext().getTypeSize(ThisPtrTy);
-      auto Align = getTypeAlignIfRequired(ThisPtrTy, CGM.getContext());
-      llvm::DIType *PointeeType =
-          getOrCreateType(ThisPtrTy->getPointeeType(), Unit);
-      llvm::DIType *ThisPtrType =
-          DBuilder.createPointerType(PointeeType, Size, Align);
-      TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
-      // TODO: This and the artificial type below are misleading, the
-      // types aren't artificial the argument is, but the current
-      // metadata doesn't represent that.
-      ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
-      Elts.push_back(ThisPtrType);
-    } else {
-      llvm::DIType *ThisPtrType = getOrCreateType(ThisPtr, Unit);
-      TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
-      ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
-      Elts.push_back(ThisPtrType);
-    }
+    llvm::DIType *ThisPtrType = getOrCreateType(ThisPtr, Unit);
+    TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
+    ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
+    Elts.push_back(ThisPtrType);
   }
 
   // Copy rest of the arguments.
@@ -3509,6 +3491,11 @@ llvm::DIType *CGDebugInfo::CreateType(const PipeType *Ty, llvm::DIFile *U) {
   return getOrCreateType(Ty->getElementType(), U);
 }
 
+llvm::DIType *CGDebugInfo::CreateType(const HLSLAttributedResourceType *Ty,
+                                      llvm::DIFile *U) {
+  return getOrCreateType(Ty->getWrappedType(), U);
+}
+
 llvm::DIType *CGDebugInfo::CreateEnumType(const EnumType *Ty) {
   const EnumDecl *ED = Ty->getDecl();
 
@@ -3851,12 +3838,14 @@ llvm::DIType *CGDebugInfo::CreateTypeNode(QualType Ty, llvm::DIFile *Unit) {
 
   case Type::TemplateSpecialization:
     return CreateType(cast<TemplateSpecializationType>(Ty), Unit);
+  case Type::HLSLAttributedResource: {
+    return CreateType(cast<HLSLAttributedResourceType>(Ty), Unit);
+  }
 
   case Type::CountAttributed:
   case Type::Auto:
   case Type::Attributed:
   case Type::BTFTagAttributed:
-  case Type::HLSLAttributedResource:
   case Type::Adjusted:
   case Type::Decayed:
   case Type::DeducedTemplateSpecialization:
diff --git a/clang/lib/CodeGen/CGDebugInfo.h b/clang/lib/CodeGen/CGDebugInfo.h
index 3fd0237a1c61dd..38f73eca561b7e 100644
--- a/clang/lib/CodeGen/CGDebugInfo.h
+++ b/clang/lib/CodeGen/CGDebugInfo.h
@@ -196,6 +196,8 @@ class CGDebugInfo {
   llvm::DIType *CreateType(const PointerType *Ty, llvm::DIFile *F);
   llvm::DIType *CreateType(const BlockPointerType *Ty, llvm::DIFile *F);
   llvm::DIType *CreateType(const FunctionType *Ty, llvm::DIFile *F);
+  llvm::DIType *CreateType(const HLSLAttributedResourceType *Ty,
+                           llvm::DIFile *F);
   /// Get structure or union type.
   llvm::DIType *CreateType(const RecordType *Tyg);
 
diff --git a/clang/test/CodeGenHLSL/debug/rwbuffer_debug_info.hlsl b/clang/test/CodeGenHLSL/debug/rwbuffer_debug_info.hlsl
new file mode 100644
index 00000000000000..705be1af08be23
--- /dev/null
+++ b/clang/test/CodeGenHLSL/debug/rwbuffer_debug_info.hlsl
@@ -0,0 +1,11 @@
+// RUN: %clang  --driver-mode=dxc -Zi -Fc - -T cs_6_3 -O0 %s | FileCheck %s
+
+// CHECK: #dbg_declare(ptr [[ThisReg:%this\..*]], [[ThisMd:![0-9]+]],
+// CHECK-DAG: [[ThisMd]] = !DILocalVariable(name: "this", arg: 1, scope: !{{[0-9]+}}, type: ![[type:[0-9]+]], flags: DIFlagArtificial | DIFlagObjectPointer)
+
+RWBuffer<float4> Out : register(u7, space4);
+
+[numthreads(8,1,1)]
+void main(uint GI : SV_GroupIndex) {
+  Out[GI] = 0;
+}

[bogner]

Is removing this entire block NFC? I'm surprised that this doesn't affect anything. If so, we should probably make the NFC change separately from the HLSL specific one so that it's clear what (isn't) changing.

[joaosaffran]

Well, this is a consequence of the this fix, the same done in DXC. After applying the fix the code becomes:

if (isa<ClassTemplateSpecializationDecl>(RD)) {
    // HLSL Change Begin - This is a reference.
    llvm::DIType *ThisPtrType = getOrCreateType(ThisPtr, Unit);
    // HLSL Change End - This is a reference.
    TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
    // TODO: This and the artificial type below are misleading, the
    // types aren't artificial the argument is, but the current
    // metadata doesn't represent that.
    ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
    Elts.push_back(ThisPtrType);
  } else {
    llvm::DIType *ThisPtrType = getOrCreateType(ThisPtr, Unit);
    TypeCache[ThisPtr.getAsOpaquePtr()].reset(ThisPtrType);
    ThisPtrType = DBuilder.createObjectPointerType(ThisPtrType);
    Elts.push_back(ThisPtrType);
  }

Removing the comments, if and else blocks become the same code, so I just removed the branching

[llvm-beanz]

This code has been around in clang for a long time, and it was implemented back when LLVM's testing standards were not quite as high as they are today (fa59ac3).

The test case that was introduced with the original code here is here, and it generates identical IR with and without this PR.

It would be nice to have someone who understands C++ debug info well review this change as well, but it looks to me like the special handling here is vestigial and no longer needed.

@dwblaikie, @adrian-prantl, or @Michael137, can one of you take a look at this please?

[dwblaikie]

huh, interesting archaeology... Wish we knew which test in particular it was.
This change might've been before I changed one of the old/core (before vtables, template specializations, and ctor homing) -flimit-debug-info thing which was more ad-hoc, to be powered by class "required to be complete" events.

So, yeah, I could totally believe whatever this code did may've been subsumed by other work/generalizations/etc since then.

but, yeah, if we're removing it, best to do it separately in an NFC change & if someone's feeling like doing due diligence, perhaps building clang with/without this change (on linux, or otherwise with -flimit-debug-info enabled) and checking the binary output is identical?

[joaosaffran]

Created a NFC with this change, please take a look here: #119445

[llvm-beanz]

nit: remove unnecessary braces.

Suggested change

	case Type::HLSLAttributedResource: {
	return CreateType(cast<HLSLAttributedResourceType>(Ty), Unit);
	}
	case Type::HLSLAttributedResource:
	return CreateType(cast<HLSLAttributedResourceType>(Ty), Unit);

[llvm-beanz]

What is the actual type this is specifying? My read is it specifies float4 which isn't really what we want. We probably want it to instead become an opaque struct pointer.