[HLSL] Do not print details in IR for target extension types

[bob80905]

This PR changes how target extension types are printed when they are emitted as IR.
This prevents repetitive phrases like "struct = type {...}" from being repeated over and over in the outputted IR.
Additionally, it should allow opt to not crash when parsing the DXIL output.

Fixes #114131

[llvmbot]

@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-backend-directx
@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-hlsl

@llvm/pr-subscribers-clang

Author: Joshua Batista (bob80905)

Changes

This PR changes the way that final DXIL is printed by adjusting the way the type definition section of the DXIL output is printed.
Specifically, when defining a target extension type, if the RHS involves a struct type, then rather than printing the entire struct type definition, only the struct body is printed.
This prevents repetitive phrases like "struct = type {...}" from being repeated over and over in the RHS.
Additionally, it should allow opt to not crash when parsing the DXIL output.

Fixes #114131

---

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

2 Files Affected:

• (added) clang/test/AST/HLSL/StructuredBuffer-AST-2.hlsl (+30)
• (modified) llvm/lib/IR/AsmWriter.cpp (+8-2)

diff --git a/clang/test/AST/HLSL/StructuredBuffer-AST-2.hlsl b/clang/test/AST/HLSL/StructuredBuffer-AST-2.hlsl
new file mode 100644
index 00000000000000..73073b3f6f2839
--- /dev/null
+++ b/clang/test/AST/HLSL/StructuredBuffer-AST-2.hlsl
@@ -0,0 +1,30 @@
+// RUN: %clang_dxc -T cs_6_6 %s | FileCheck %s
+
+// The purpose of this test is to ensure that the AST writer
+// only emits struct bodies when within the context of a 
+// larger object that is being outputted on the RHS.
+
+
+// note that "{ <4 x float> }" in the check below is a struct type, but only the
+// body is emitted on the RHS because we are already in the context of a
+// target extension type definition (class.hlsl::StructuredBuffer)
+// CHECK: %"class.hlsl::StructuredBuffer" = type { target("dx.RawBuffer", { <4 x float> }, 0, 0), %struct.mystruct }
+// CHECK: %struct.mystruct = type { <4 x float> }
+// CHECK: %dx.types.Handle = type { ptr }
+// CHECK: %dx.types.ResBind = type { i32, i32, i32, i8 }
+// CHECK: %dx.types.ResourceProperties = type { i32, i32 }
+
+struct mystruct
+{
+    float4 Color;
+};
+
+StructuredBuffer<mystruct> my_buffer : register(t2, space4);
+
+export float4 test()
+{
+    return my_buffer[0].Color;
+}
+
+[numthreads(1,1,1)]
+void main() {}
diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
index 3c1cb76622bbb7..d2705ff4b30fb8 100644
--- a/llvm/lib/IR/AsmWriter.cpp
+++ b/llvm/lib/IR/AsmWriter.cpp
@@ -649,8 +649,14 @@ void TypePrinting::print(Type *Ty, raw_ostream &OS) {
     OS << "target(\"";
     printEscapedString(Ty->getTargetExtName(), OS);
     OS << "\"";
-    for (Type *Inner : TETy->type_params())
-      OS << ", " << *Inner;
+    for (Type *Inner : TETy->type_params()) {
+      OS << ", ";
+      if (Inner->isStructTy()) {
+        StructType *STy = cast<StructType>(Inner);
+        printStructBody(STy, OS);
+      } else
+        OS << *Inner;
+    }
     for (unsigned IntParam : TETy->int_params())
       OS << ", " << IntParam;
     OS << ")";

[bogner]

This doesn't look right to me. Changing the type printer so it doesn't differentiate between named and anonymous structs inside a target extension type like this looks like it will break round-tripping through LLVM IR, since the fact that this was associated with a named struct and not an anonymous one is lost.

I suspect we're actually creating the wrong types (in DirectXTargetCodeGenInfo::getHLSLType) in the first place, since target extension types don't seem to handle named structs at all. Additionally, we should probably add checks when creating a target extension type to prevent creating them with named struct types.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[bogner]

This isn't testing the AST, so it shouldn't be in the test/AST folder. In any case, I think there's sufficient coverage by the changes to the CodeGenHLSL/builtins tests and TypesTest, so this one can probably just be removed.

[bogner]

No need for a vector here - this is a fixed size array. Also what's "original" about these elements?

Suggested change

	std::vector<Type *> OriginalElements = {Int32Type, FloatType};
	std::array<Type *, 2> Elements{Int32Type, FloatType};

[bogner]

There's an overload of create that sets the elements directly:

Suggested change

	StructType *Struct = llvm::StructType::create(Context, "MyStruct");
	Struct->setBody(OriginalElements);
	StructType *Struct = llvm::StructType::create(Context, Elements, "MyStruct",
	/*isPacked=*/false);

[bogner]

The StructType docs call an unnamed struct a "literal struct" rather than anonymous: https://llvm.org/doxygen/classllvm_1_1StructType.html#details.

I would probably just say something to the effect of "ensure that literal structs in the target extension type print the struct body" to mirror the comment earlier.

[bogner]

It'd be better to do all of the work to test the identified struct, and then do the work for the literal struct as a second step. This way you can avoid the duplicate StructType *, TargetExtensionType *, SmallVector, and raw_svector_ostream variables, and it will be clearer that these are two related but entirely independent tests.

[bogner]

Looks good!

[bogner]

It would be fine to reuse the variables from above here (Struct, TargetExtensionType, TETV, TETStream)

[bogner]

The title and description are out of date since we changed the approach here - please update them.

[hekota]

LGTM!