Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Dec 10, 2024
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
7413ceb
print struct body within target ext ty context
bob80905 Nov 13, 2024
69d90f2
fix runline
bob80905 Nov 13, 2024
3a1bdba
undo change
bob80905 Nov 13, 2024
c3e6120
convert named to anonymous structs when creating target ext types
bob80905 Nov 22, 2024
0d2579a
clang-format
bob80905 Nov 22, 2024
b24c6d7
add assert, move conversion
bob80905 Nov 22, 2024
e3f9344
change asmwriter
bob80905 Nov 22, 2024
97b0745
add test to typestest.cpp
bob80905 Nov 23, 2024
c68234e
update tests
bob80905 Nov 25, 2024
e4c9bf5
address Justin
bob80905 Nov 25, 2024
a612ace
Merge branch 'main' into ast_print_struct_body_in_higher_context
bob80905 Dec 9, 2024
ac26aa6
update tests
bob80905 Dec 9, 2024
e6a9219
clang format
bob80905 Dec 9, 2024
e013c09
fix test
bob80905 Dec 9, 2024
2c37c3c
remove redeclaration
bob80905 Dec 9, 2024
907465b
remove bad clear call
bob80905 Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions clang/test/AST/HLSL/StructuredBuffer-AST-2.hlsl
View file
Open in desktop
Copy link
Contributor

@bogner bogner Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.6-compute -emit-llvm -finclude-default-header -o - %s | FileCheck %s

// The purpose of this test is to ensure that the target
// extension type associated with the structured buffer only
// contains anonymous struct types, rather than named
// struct types

// 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", %struct.mystruct, 0, 0), %struct.mystruct }
// CHECK: %struct.mystruct = type { <4 x float> }

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() {}
3 changes: 2 additions & 1 deletion clang/test/CodeGenHLSL/builtins/AppendStructuredBuffer-elementtype.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ struct MyStruct {
// DXIL: %"class.hlsl::AppendStructuredBuffer.9" = type { target("dx.RawBuffer", <3 x i32>, 1, 0)
// DXIL: %"class.hlsl::AppendStructuredBuffer.10" = type { target("dx.RawBuffer", <2 x half>, 1, 0)
// DXIL: %"class.hlsl::AppendStructuredBuffer.11" = type { target("dx.RawBuffer", <3 x float>, 1, 0)
// DXIL: %"class.hlsl::AppendStructuredBuffer.12" = type { target("dx.RawBuffer", %struct.MyStruct = type { <4 x float>, <2 x i32>, [8 x i8] }, 1, 0)
// DXIL: %"class.hlsl::AppendStructuredBuffer.12" = type { target("dx.RawBuffer", %struct.MyStruct, 1, 0)
// DXIL: %struct.MyStruct = type { <4 x float>, <2 x i32>, [8 x i8] }

AppendStructuredBuffer<int16_t> BufI16;
AppendStructuredBuffer<uint16_t> BufU16;
Expand Down
3 changes: 2 additions & 1 deletion clang/test/CodeGenHLSL/builtins/ConsumeStructuredBuffer-elementtype.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ struct MyStruct {
// DXIL: %"class.hlsl::ConsumeStructuredBuffer.9" = type { target("dx.RawBuffer", <3 x i32>, 1, 0)
// DXIL: %"class.hlsl::ConsumeStructuredBuffer.10" = type { target("dx.RawBuffer", <2 x half>, 1, 0)
// DXIL: %"class.hlsl::ConsumeStructuredBuffer.11" = type { target("dx.RawBuffer", <3 x float>, 1, 0)
// DXIL: %"class.hlsl::ConsumeStructuredBuffer.12" = type { target("dx.RawBuffer", %struct.MyStruct = type { <4 x float>, <2 x i32>, [8 x i8] }, 1, 0)
// DXIL: %"class.hlsl::ConsumeStructuredBuffer.12" = type { target("dx.RawBuffer", %struct.MyStruct, 1, 0)
// DXIL: %struct.MyStruct = type { <4 x float>, <2 x i32>, [8 x i8] }

ConsumeStructuredBuffer<int16_t> BufI16;
ConsumeStructuredBuffer<uint16_t> BufU16;
Expand Down
3 changes: 2 additions & 1 deletion clang/test/CodeGenHLSL/builtins/RasterizerOrderedStructuredBuffer-elementtype.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ struct MyStruct {
// DXIL: %"class.hlsl::RasterizerOrderedStructuredBuffer.11" = type { target("dx.RawBuffer", <3 x i32>, 1, 1)
// DXIL: %"class.hlsl::RasterizerOrderedStructuredBuffer.12" = type { target("dx.RawBuffer", <2 x half>, 1, 1)
// DXIL: %"class.hlsl::RasterizerOrderedStructuredBuffer.13" = type { target("dx.RawBuffer", <3 x float>, 1, 1)
// DXIL: %"class.hlsl::RasterizerOrderedStructuredBuffer.14" = type { target("dx.RawBuffer", %struct.MyStruct = type { <4 x float>, <2 x i32>, [8 x i8] }, 1, 1)
// DXIL: %"class.hlsl::RasterizerOrderedStructuredBuffer.14" = type { target("dx.RawBuffer", %struct.MyStruct, 1, 1)
// DXIL: %struct.MyStruct = type { <4 x float>, <2 x i32>, [8 x i8] }

RasterizerOrderedStructuredBuffer<int16_t> BufI16;
RasterizerOrderedStructuredBuffer<uint16_t> BufU16;
Expand Down
3 changes: 2 additions & 1 deletion clang/test/CodeGenHLSL/builtins/hlsl_resource_t.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
using handle_float_t = __hlsl_resource_t [[hlsl::resource_class(UAV)]] [[hlsl::contained_type(float)]];

// CHECK: %"class.hlsl::RWBuffer" = type { target("dx.TypedBuffer", <4 x float>, 1, 0, 0)
// CHECK: %"class.hlsl::StructuredBuffer" = type { target("dx.RawBuffer", %struct.MyStruct = type { <4 x float>, <2 x i32>, [8 x i8] }, 0, 0)
// CHECK: %"class.hlsl::StructuredBuffer" = type { target("dx.RawBuffer", %struct.MyStruct, 0, 0)
// CHECK: %struct.MyStruct = type { <4 x float>, <2 x i32>, [8 x i8] }

// CHECK: define void @_Z2faU9_Res_u_CTfu17__hlsl_resource_t(target("dx.TypedBuffer", float, 1, 0, 0) %a)
// CHECK: call void @_Z4foo1U9_Res_u_CTfu17__hlsl_resource_t(target("dx.TypedBuffer", float, 1, 0, 0) %0)
Expand Down
6 changes: 4 additions & 2 deletions llvm/lib/IR/AsmWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,8 +649,10 @@ 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 << ", ";
Inner->print(OS, /*IsForDebug=*/false, /*NoDetails=*/true);
}
for (unsigned IntParam : TETy->int_params())
OS << ", " << IntParam;
OS << ")";
Expand Down
33 changes: 33 additions & 0 deletions llvm/unittests/IR/TypesTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,43 @@ TEST(TypesTest, TargetExtType) {
Type *A = TargetExtType::get(Context, "typea");
Type *Aparam = TargetExtType::get(Context, "typea", {}, {0, 1});
Type *Aparam2 = TargetExtType::get(Context, "typea", {}, {0, 1});

// Opaque types with same parameters are identical...
EXPECT_EQ(Aparam, Aparam2);
// ... but just having the same name is not enough.
EXPECT_NE(A, Aparam);

// ensure struct types in targest extension types
// only show the struct name, not the struct body
Type *Int32Type = Type::getInt32Ty(Context);
Type *FloatType = Type::getFloatTy(Context);
std::vector<Type *> OriginalElements = {Int32Type, FloatType};
Copy link
Contributor

@bogner bogner Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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};

StructType *Struct = llvm::StructType::create(Context, "MyStruct");
Struct->setBody(OriginalElements);
Copy link
Contributor

@bogner bogner Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);


// the other struct is different only in that it's an anonymous struct,
// without a name
Copy link
Contributor

@bogner bogner Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

StructType *OtherStruct =
StructType::get(Context, Struct->elements(), /*isPacked=*/false);

Type *TargetExtensionType =
TargetExtType::get(Context, "structTET", {Struct}, {0, 1});
Type *OtherTargetExtensionType =
TargetExtType::get(Context, "structTET", {OtherStruct}, {0, 1});

SmallVector<char, 50> TETV;
SmallVector<char, 50> OtherTETV;

llvm::raw_svector_ostream TETStream(TETV);
TargetExtensionType->print(TETStream);

llvm::raw_svector_ostream OtherTETStream(OtherTETV);
OtherTargetExtensionType->print(OtherTETStream);

EXPECT_STREQ(TETStream.str().str().data(),
"target(\"structTET\", %MyStruct, 0, 1)");
EXPECT_STREQ(OtherTETStream.str().str().data(),
"target(\"structTET\", { i32, float }, 0, 1)");
Copy link
Contributor

@bogner bogner Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

TEST(TypedPointerType, PrintTest) {
Expand Down
Loading