tests: Improvements for SM1 testing.
This is the first part of !608 (merged), which I decided to separate to ease review.
It consists of improvements to SM1 testing, mainly:
- Correctly passing int and bool uniforms as IEEE 754 floats to SM1 profiles.
- The introduction of the "only" qualifier to avoid abusing
[require]
blocks in case tests retrieve different results for different shader models. - Allowing some of the tests we already have for SM4/SM6 to run in SM1 after those improvements.
Merge request reports
Activity
mentioned in merge request !608 (merged)
Part of the point of passing bools as a value other than 1 or 0 there is that it validated that sm4 was doing the right thing in flushing them to 1 and 0. Why not just flush for sm1 in the shader runner like in 3/5?
Also, regardless, I don't think we need a separate "bool" type, we can just use int or uint.
Yeah, I mostly see the interface between the CPU program and the shaders as binary, not typed. Specifying a type in the
.shader_test
file is a convenience for humans, but I don't think there should be any magic happening. Rather, I'd use theonly()
syntax you just introduced to specify different inputs depending on the SM:only(sm<4) uniform 0 float4 1.0 2.0 3.0 4.0 only(sm>=4) uniform 0 uint4 1 2 3 4
How does that feel?
Since we will be passing ints using the "only" qualifier as Giovanni suggested, we can now just provide different offsets for the different shader models.
So something like this:
uniform 0 int -1 uniform 1 uint 3 uniform 2 int -2 uniform 3 float 0.5
ends ups like this:
if(sm<4) uniform 0 float -1 if(sm<4) uniform 4 float 3 if(sm<4) uniform 8 float 1 if(sm<4) uniform 12 float 0.5 if(sm>=4) uniform 0 int -1 if(sm>=4) uniform 1 uint 3 if(sm>=4) uniform 2 int -2 if(sm>=4) uniform 3 float 0.5
From 78ecef0ccf024fa728c952c13cf0c885c61b3a6f Mon Sep 17 00:00:00 2001 From: Francisco Casas <fcasas@codeweavers.com> Date: Mon, 29 Jan 2024 20:07:39 -0300 Subject: [PATCH 2/5] tests/shader-runner: Introduce "only" qualifier. When the "only" qualifier is added to a directive, the directive is skipped if the shader->minimum_shader_model is not included in the range. This can be used on the "probe" directives for tests that have different expected results on different shader models, without having to resort to [require] blocks.
I think most languages call that "if"? I.e.:
if(sm<6) probe ... if(sm>=6) probe ...
Not that I mind doing something different, but I suppose it might make things slightly harder to pick up for someone new to writing .shader_test's.
Some possible alternatives:
probe(sm<6) ... probe(sm>=6) ...
cond (sm<6) probe ... (sm>=6) probe ...
added 4 commits
Toggle commit listI updated according to the suggestions.
There are two conflicting ideas of how to pass uniform values to the backend:
a) Allow the shader_runner to perform transformations of the input values according to the backend and the type provided (so it makes sense to add the "bool" type to the "uniform" directives)
b) Don't allow the shader_runner to perform these transformations, and instead pass the values as if they where binary data, needing the use of more "only" (now called "if") qualifiers.
The first version of this MR was doing (a) but the current one does (b). The disadvantage of (b) is that we have to add more "if" qualifiers, if you think these are too many I can revert to (a).
Edited by Francisco Casas@@ -540,9 +540,12 @@ static void set_resource(struct shader_runner *runner, struct resource *resource static void set_uniforms(struct shader_runner *runner, size_t offset, size_t count, const void *uniforms) { + size_t initial_capacity = runner->uniform_capacity; + runner->uniform_count = align(max(runner->uniform_count, offset + count), 4); vkd3d_array_reserve((void **)&runner->uniforms, &runner->uniform_capacity, runner->uniform_count, sizeof(*runner->uniforms)); + memset(runner->uniforms + initial_capacity, 0, runner->uniform_capacity - initial_capacity); memcpy(runner->uniforms + offset, uniforms, count * sizeof(*runner->uniforms)); }
How does that work? Are we sending more than "uniform_count" uniforms to the shader anywhere? Or is this about the potential padding introduced by align? In the latter case, we need to clear a fair bit less than we do here. We may also want to consider clearing with a value other than zero.
The first version of this MR was doing (a) but the current one does (b). The disadvantage of (b) is that we have to add more "if" qualifiers, if you think these are too many I can revert to (a).
We could conceivably introduce additional types that do type conversion when targetting d3dbc if the current scheme becomes too cumbersome, but for the moment this seems fine.
How does that work? Are we sending more than "uniform_count" uniforms to the shader anywhere? Or is this about the potential padding introduced by align?
Both in SM1 or when we work with arrays we commonly declare uniforms leaving the padding uninitialized:
uniform 0 float 1.0 uniform 4 float 2.0 uniform 8 float 3.0
so the aim is to initialize it, including the padding after the last uniform declared.
My idea was to initialize the new values in the array after a reallocation happens, before inserting the values provided at the offset position, to ensure that we initialize the padding.
In the latter case, we need to clear a fair bit less than we do here.
Ah, I see it now.
We may also want to consider clearing with a value other than zero.
Yep, I think it makes sense to initialize to a sentry value other than zero, maybe 127 ?
btw, I forgot to multiply the amount of bytes by the size of each uniform. So the final instruction should be:
memset(runner->uniforms + initial_count, 127, (runner->uniform_count - initial_count) * sizeof(*runner->uniforms));
Edited by Francisco Casas
added 4 commits
Toggle commit listadded 79 commits
-
70b13227...5eba031f - 75 commits from branch
wine:master
- dbf614c5 - tests/vkd3d-shader: Set uninitialized uniforms to zero.
- 65f44896 - tests: Don't ignore SM1 on a non-const-indexing.shader_test test.
- a01c80c3 - tests/shader-runner: Introduce "if" qualifier.
- d8398170 - tests: Remove [require] directives for tests that use int and bool uniforms.
Toggle commit list-
70b13227...5eba031f - 75 commits from branch
added 4 commits
Toggle commit list@@ -545,15 +572,17 @@ float4 main() : sv_target } [test] -uniform 0 uint4 1 0 3 1 +if(sm<4) uniform 0 float4 0 0 3 1 +if(sm>=4) uniform 0 uint4 0 0 3 1 draw quad -probe all rgba (10.0, 11.0, 12.0, 13.0) -uniform 0 uint4 0 0 3 1 +todo(sm>=4) probe all rgba (10.0, 11.0, 12.0, 13.0) +if(sm<4) uniform 0 float4 1 0 3 1 +if(sm>=4) uniform 0 uint4 1 0 3 1
That looks like a rebase glitch after 4ba324d3.
added 1 commit
- d172e78d - tests: Remove [require] directives for tests that use int and bool uniforms.
added 1 commit
- 9cb6eb9c - tests: Remove [require] directives for tests that use int and bool uniforms.
added 4 commits
Toggle commit list