-
Notifications
You must be signed in to change notification settings - Fork 677
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.
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
Ready tests for 6.9 version #6465
base: main
Are you sure you want to change the base?
Conversation
node shader validation tests depended heavily on metadata numbers and order. This make them robust so they can work with the 6.9 changes or without
deduplication of the 1,9 tuple made the test fail. Using -DAG checks allows the order to work anyway
You can test this locally with the following command:git-clang-format --diff e4fa1c518413a3ca871a1b1699308b6db0ede892 b94408dcb6dbbcb6fa84d4efb1440b45280656a1 -- tools/clang/unittests/HLSL/ValidationTest.cpp View the diff from clang-format here.diff --git a/tools/clang/unittests/HLSL/ValidationTest.cpp b/tools/clang/unittests/HLSL/ValidationTest.cpp
index 26bc6e88..aeb83571 100644
--- a/tools/clang/unittests/HLSL/ValidationTest.cpp
+++ b/tools/clang/unittests/HLSL/ValidationTest.cpp
@@ -4274,7 +4274,7 @@ TEST_F(ValidationTest, ComputeNodeCompatibility) {
{"!\"node01\", null, null, !([0-9]+)}\n"
"!\\1 = !{i32 8, i32 15"}, // original: node shader
{"!\"node01\", null, null, !\\1}\n"
- "!\\1 = !{i32 8, i32 5"}, // changed to: compute shader
+ "!\\1 = !{i32 8, i32 5"}, // changed to: compute shader
"Compute entry 'node01' has unexpected node shader metadata", true);
RewriteAssemblyCheckMsg(
L"..\\DXILValidation\\compute_node_compatibility.hlsl", "lib_6_8",
@@ -4282,7 +4282,7 @@ TEST_F(ValidationTest, ComputeNodeCompatibility) {
{"!\"node02\", null, null, !([0-9]+)}\n"
"!\\1 = !{i32 8, i32 15"}, // original: node shader
{"!\"node02\", null, null, !\\1}\n"
- "!\\1 = !{i32 8, i32 5"}, // changed to: compute shader
+ "!\\1 = !{i32 8, i32 5"}, // changed to: compute shader
"Compute entry 'node02' has unexpected node shader metadata", true);
RewriteAssemblyCheckMsg(
L"..\\DXILValidation\\compute_node_compatibility.hlsl", "lib_6_8",
@@ -4290,7 +4290,7 @@ TEST_F(ValidationTest, ComputeNodeCompatibility) {
{"!\"node03\", null, null, !([0-9]+)}\n"
"!\\1 = !{i32 8, i32 15"}, // original: node shader
{"!\"node03\", null, null, !\\1}\n"
- "!\\1 = !{i32 8, i32 5"}, // changed to: compute shader
+ "!\\1 = !{i32 8, i32 5"}, // changed to: compute shader
"Compute entry 'node03' has unexpected node shader metadata", true);
RewriteAssemblyCheckMsg(
L"..\\DXILValidation\\compute_node_compatibility.hlsl", "lib_6_8",
@@ -4298,7 +4298,7 @@ TEST_F(ValidationTest, ComputeNodeCompatibility) {
{"!\"node04\", null, null, !([0-9]+)}\n"
"!\\1 = !{i32 8, i32 15"}, // original: node shader
{"!\"node04\", null, null, !\\1}\n"
- "!\\1 = !{i32 8, i32 5"}, // changed to: compute shader
+ "!\\1 = !{i32 8, i32 5"}, // changed to: compute shader
"Compute entry 'node04' has unexpected node shader metadata", true);
RewriteAssemblyCheckMsg(
L"..\\DXILValidation\\compute_node_compatibility.hlsl", "lib_6_8",
@@ -4307,8 +4307,8 @@ TEST_F(ValidationTest, ComputeNodeCompatibility) {
"!\\1 = !{i32 8, i32 15, i32 13, i32 2"}, // original: node shader
// coalesing launch type
{"!\"node04\", null, null, !\\1}\n"
- "!\\1 = !{i32 8, i32 5, i32 13, i32 3"}, // changed to: compute shader
- // thread launch type
+ "!\\1 = !{i32 8, i32 5, i32 13, i32 3"}, // changed to: compute shader
+ // thread launch type
"Compute entry 'node04' has unexpected node shader metadata", true);
}
@@ -4419,11 +4419,13 @@ TEST_F(ValidationTest, NodeInputMultiplicity) {
"!\\2 = !{!([0-9]+)}", // input records
"= !{i32 1, i32 33, i32 2, !([0-9]+)}"}, // end of output
{"= !{i32 8, i32 15, i32 13, i32 1,\\1i32 20, !\\2\\3"
- "!\\2 = !{!\\4, !100}", // multiple input records
+ "!\\2 = !{!\\4, !100}", // multiple input records
"= !{i32 1, i32 33, i32 2, !\\1}\n"
- "!100 = !{i32 1, i32 97, i32 2, !\\1}\n"}, // extra DispatchNodeInputRecord
+ "!100 = !{i32 1, i32 97, i32 2, !\\1}\n"}, // extra
+ // DispatchNodeInputRecord
"node shader 'node01' may not have more than one input record \\(2 are "
- "declared\\)", true);
+ "declared\\)",
+ true);
RewriteAssemblyCheckMsg(
L"..\\DXILValidation\\node_input_compatibility.hlsl", "lib_6_8",
@@ -4432,11 +4434,12 @@ TEST_F(ValidationTest, NodeInputMultiplicity) {
"!\\2 = !{!([0-9]+)}", // input records
"= !{i32 1, i32 33, i32 2, !([0-9]+)}"}, // end of output
{"= !{i32 8, i32 15, i32 13, i32 1,\\1i32 20, !\\2\\3"
- "!\\2 = !{!\\4, !100}", // multiple input records
+ "!\\2 = !{!\\4, !100}", // multiple input records
"= !{i32 1, i32 33, i32 2, !\\1}\n"
"!100 = !{i32 1, i32 9, i32 2, !\\1}\n"}, // extra EmptyNodeInput
"node shader 'node01' may not have more than one input record \\(2 are "
- "declared\\)", true);
+ "declared\\)",
+ true);
RewriteAssemblyCheckMsg(
L"..\\DXILValidation\\node_input_compatibility.hlsl", "lib_6_8",
@@ -4445,11 +4448,12 @@ TEST_F(ValidationTest, NodeInputMultiplicity) {
"!\\2 = !{!([0-9]+)}", // input records
"= !{i32 1, i32 33, i32 2, !([0-9]+)}"}, // end of output
{"= !{i32 8, i32 15, i32 13, i32 2,\\1i32 20, !\\2\\3"
- "!\\2 = !{!\\4, !100}", // multiple input records
+ "!\\2 = !{!\\4, !100}", // multiple input records
"= !{i32 1, i32 33, i32 2, !\\1}\n"
"!100 = !{i32 1, i32 65, i32 2, !\\1}\n"}, // extra GroupNodeInputRecords
"node shader 'node02' may not have more than one input record \\(2 are "
- "declared\\)", true);
+ "declared\\)",
+ true);
RewriteAssemblyCheckMsg(
L"..\\DXILValidation\\node_input_compatibility.hlsl", "lib_6_8",
@@ -4458,11 +4462,12 @@ TEST_F(ValidationTest, NodeInputMultiplicity) {
"!\\2 = !{!([0-9]+)}", // input records
"= !{i32 1, i32 33, i32 2, !([0-9]+)}"}, // end of output
{"= !{i32 8, i32 15, i32 13, i32 2,\\1i32 20, !\\2\\3"
- "!\\2 = !{!\\4, !100}", // multiple input records
+ "!\\2 = !{!\\4, !100}", // multiple input records
"= !{i32 1, i32 33, i32 2, !\\1}\n"
"!100 = !{i32 1, i32 9, i32 2, !\\1}\n"}, // extra EmptyNodeInput
"node shader 'node02' may not have more than one input record \\(2 are "
- "declared\\)", true);
+ "declared\\)",
+ true);
RewriteAssemblyCheckMsg(
L"..\\DXILValidation\\node_input_compatibility.hlsl", "lib_6_8",
@@ -4471,11 +4476,12 @@ TEST_F(ValidationTest, NodeInputMultiplicity) {
"!\\2 = !{!([0-9]+)}", // input records
"= !{i32 1, i32 33, i32 2, !([0-9]+)}"}, // end of output
{"= !{i32 8, i32 15, i32 13, i32 3,\\1i32 20, !\\2\\3"
- "!\\2 = !{!\\4, !100}", // multiple input records
+ "!\\2 = !{!\\4, !100}", // multiple input records
"= !{i32 1, i32 33, i32 2, !\\1}\n"
"!100 = !{i32 1, i32 33, i32 2, !\\1}\n"}, // extra ThreadNodeInputRecord
"node shader 'node03' may not have more than one input record \\(2 are "
- "declared\\)", true);
+ "declared\\)",
+ true);
RewriteAssemblyCheckMsg(
L"..\\DXILValidation\\node_input_compatibility.hlsl", "lib_6_8",
@@ -4484,12 +4490,12 @@ TEST_F(ValidationTest, NodeInputMultiplicity) {
"!\\2 = !{!([0-9]+)}", // input records
"= !{i32 1, i32 33, i32 2, !([0-9]+)}"}, // end of output
{"= !{i32 8, i32 15, i32 13, i32 3,\\1i32 20, !\\2\\3"
- "!\\2 = !{!\\4, !100}", // multiple input records
+ "!\\2 = !{!\\4, !100}", // multiple input records
"= !{i32 1, i32 33, i32 2, !\\1}\n"
"!100 = !{i32 1, i32 9, i32 2, !\\1}\n"}, // extra EmptyNodeInput
"node shader 'node03' may not have more than one input record \\(2 are "
- "declared\\)", true);
-
+ "declared\\)",
+ true);
}
TEST_F(ValidationTest, CacheInitWithMinPrec) {
|
@@ -4310,88 +4320,88 @@ TEST_F(ValidationTest, NodeInputCompatibility) { | |||
RewriteAssemblyCheckMsg( | |||
L"..\\DXILValidation\\node_input_compatibility.hlsl", "lib_6_8", | |||
pArguments.data(), 2, nullptr, 0, | |||
{"!13 = !{i32 1, i32 97"}, // DispatchNodeInputRecord | |||
{"!13 = !{i32 1, i32 65"}, // GroupNodeInputRecords | |||
{"= !{i32 1, i32 97"}, // DispatchNodeInputRecord |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this uses a different pattern to the ones above that were verifying that the thing on the left of the =
matched?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If by the above, you mean the pattern matching in ComputeNodeCompatibility, that one requires repeating some of the metadata references in the replacement text, so they need to be captured. In this case, we're just changing an integer on the right side of the equation. The shader is simpler and this is sufficient to match it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, that's true of the below, but not so much the above. I suppose it could be simpler there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've reminded myself that in the above case, we have several node entries that have the same metadata. To distinguish and alter them independently, we need to bring in more information including the metadata that identifies the function in question.
Without this level of specificity, each replacement will alter the shader or launch type data for all matching disassembly. That happens to work, but it's not really what's meant to be tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we need to copy any updates to these changes back into the mesh nodes branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no unique updates here. These commits are all copied from those that were part of #6436. Nothing has changed. This is the copy from there to here.
tools/clang/test/CodeGenDXIL/hlsl/objects/NodeObjects/node-objects-metadata.hlsl
Show resolved
Hide resolved
@@ -86,7 +86,7 @@ void node02() | |||
|
|||
// expected-note@+2 {{declared here}} | |||
[Shader("vertex")] | |||
float4 vs(float2 a :A) :SV_POSTION { | |||
float4 vs(float2 a :A) :SV_Position { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened here? Did this cause some sort of failure? The original should have been equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original misspelled "POSITION". It was an incidental change. I altered the capitalization as I think it makes the error easier to spot.
@@ -4310,88 +4320,88 @@ TEST_F(ValidationTest, NodeInputCompatibility) { | |||
RewriteAssemblyCheckMsg( | |||
L"..\\DXILValidation\\node_input_compatibility.hlsl", "lib_6_8", | |||
pArguments.data(), 2, nullptr, 0, | |||
{"!13 = !{i32 1, i32 97"}, // DispatchNodeInputRecord | |||
{"!13 = !{i32 1, i32 65"}, // GroupNodeInputRecords | |||
{"= !{i32 1, i32 97"}, // DispatchNodeInputRecord |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we need to copy any updates to these changes back into the mesh nodes branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tex3d, if you're satisfied with my answers, please resolve the conversations.
tools/clang/test/CodeGenDXIL/hlsl/objects/NodeObjects/node-objects-metadata.hlsl
Show resolved
Hide resolved
@@ -86,7 +86,7 @@ void node02() | |||
|
|||
// expected-note@+2 {{declared here}} | |||
[Shader("vertex")] | |||
float4 vs(float2 a :A) :SV_POSTION { | |||
float4 vs(float2 a :A) :SV_Position { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original misspelled "POSITION". It was an incidental change. I altered the capitalization as I think it makes the error easier to spot.
@@ -4310,88 +4320,88 @@ TEST_F(ValidationTest, NodeInputCompatibility) { | |||
RewriteAssemblyCheckMsg( | |||
L"..\\DXILValidation\\node_input_compatibility.hlsl", "lib_6_8", | |||
pArguments.data(), 2, nullptr, 0, | |||
{"!13 = !{i32 1, i32 97"}, // DispatchNodeInputRecord | |||
{"!13 = !{i32 1, i32 65"}, // GroupNodeInputRecords | |||
{"= !{i32 1, i32 97"}, // DispatchNodeInputRecord |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no unique updates here. These commits are all copied from those that were part of #6436. Nothing has changed. This is the copy from there to here.
In experimenting with bumping the max shader model version in the mesh nodes branch, many unrelated tests started failing due to being overly-sensitive to ordering, particularly for metadata. This uses regular expressions or order-independent checks to make these tests more robust so they will be able to handle the change to 6.9 when it comes to main, whatever form that takes.
Followup to #6432