[PATCH v2 0/4] MR418: tests/shader-runner: finer control over model ranges and [shader] directive execution for backends, part 1.
First part of a series that allows the shader-runner to test SM1 compilation on [shader] directives even if a backend to run these tests is not available and prepare they way for SM1-only tests.
Specifically: - Compile [shader] directives separately from backend execution (on part 1). - Allow specifying more detailed shader ranges for todo(), fail() and notimpl() qualifiers (on part 2). - Always test shader compilation with SM1 profiles (on part 2).
The `minimum_shader_model` and `maximum_shader_model` parameters for run_shader_tests() are interpreted in the following way:
Ask the pertaining backend to run the shader_test file so that each test is executed with the lowest profile it can support within the range of shader models specified, if any.
This allows us to control how many shader models we want to test for each backend, e.g.: ``` run_shader_tests(..., SHADER_MODEL_2_0, SHADER_MODEL_3_0) run_shader_tests(..., SHADER_MODEL_4_0, SHADER_MODEL_5_1) run_shader_tests(..., SHADER_MODEL_6_0, SHADER_MODEL_6_0) ``` versus ``` for (i = SHADER_MODEL_2_0, i <= SHADER_MODEL_6_0, ++i) run_shader_tests(..., i, i); ```
Also, to allow to compile [shader] directives, which are not backend-specific, separately from the [test] directives, which are, the run_shader_tests() function is modified to skip backend specific directives if the shader_runner_ops is NULL.
Following patches are on my [master6i](https://gitlab.winehq.org/fcasas/vkd3d/-/commits/master6i) branch.
-- v2: tests/shader-runner: Compile [shader] directives separately from backend execution. tests/shader-runner: Decouple run_shader_tests() ranges from those supported by the backend. tests/shader-runner: Discern between the minimum_shader_model and the selected_shader_model. tests/shader-runner: Add missing requirement checks for backends.
From: Francisco Casas fcasas@codeweavers.com
We need these checks to properly handle tests that require target profiles 3.0 and lower, or tests that require target profile 6.0 and higher.
Since all backeds require an ops->check_requirements now, expect it to always be defined. --- tests/shader_runner.c | 3 ++- tests/shader_runner.h | 4 ++-- tests/shader_runner_d3d11.c | 13 +++++++++++++ tests/shader_runner_d3d12.c | 14 ++++++++++++++ tests/shader_runner_vulkan.c | 13 +++++++++++++ 5 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/tests/shader_runner.c b/tests/shader_runner.c index 59c1408c4..77ee63dd0 100644 --- a/tests/shader_runner.c +++ b/tests/shader_runner.c @@ -1073,8 +1073,9 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o break;
case STATE_REQUIRE: + assert(runner->ops->check_requirements); if (runner->maximum_shader_model < runner->minimum_shader_model - || (runner->ops->check_requirements && !runner->ops->check_requirements(runner))) + || !runner->ops->check_requirements(runner)) { skip_tests = true; } diff --git a/tests/shader_runner.h b/tests/shader_runner.h index 3efbcf362..f6ef44a71 100644 --- a/tests/shader_runner.h +++ b/tests/shader_runner.h @@ -140,9 +140,9 @@ struct shader_runner
struct shader_runner_ops { - /* Returns false if unable to run the given tests. If NULL, all tests are - * run. */ + /* Returns false if unable to run the given tests. */ bool (*check_requirements)(struct shader_runner *runner); + struct resource *(*create_resource)(struct shader_runner *runner, const struct resource_params *params); void (*destroy_resource)(struct shader_runner *runner, struct resource *resource); bool (*draw)(struct shader_runner *runner, D3D_PRIMITIVE_TOPOLOGY primitive_topology, unsigned int vertex_count); diff --git a/tests/shader_runner_d3d11.c b/tests/shader_runner_d3d11.c index bd9d363ce..7a8f8dac0 100644 --- a/tests/shader_runner_d3d11.c +++ b/tests/shader_runner_d3d11.c @@ -100,6 +100,18 @@ static ID3D10Blob *compile_shader(const struct d3d11_shader_runner *runner, cons return blob; }
+static bool d3d11_runner_check_requirements(struct shader_runner *r) +{ + struct d3d11_shader_runner *runner = d3d11_shader_runner(r); + + if (runner->r.maximum_shader_model < SHADER_MODEL_4_0) + return false; + if (runner->r.minimum_shader_model >= SHADER_MODEL_6_0) + return false; + + return true; +} + static IDXGIAdapter *create_adapter(void) { IDXGIFactory4 *factory4; @@ -724,6 +736,7 @@ static void d3d11_runner_release_readback(struct shader_runner *r, struct resour
static const struct shader_runner_ops d3d11_runner_ops = { + .check_requirements = d3d11_runner_check_requirements, .create_resource = d3d11_runner_create_resource, .destroy_resource = d3d11_runner_destroy_resource, .dispatch = d3d11_runner_dispatch, diff --git a/tests/shader_runner_d3d12.c b/tests/shader_runner_d3d12.c index 8453617ef..c54cb5384 100644 --- a/tests/shader_runner_d3d12.c +++ b/tests/shader_runner_d3d12.c @@ -96,6 +96,19 @@ static ID3D10Blob *compile_shader(const struct d3d12_shader_runner *runner, cons return blob; }
+static bool d3d12_runner_check_requirements(struct shader_runner *r) +{ + struct d3d12_shader_runner *runner = d3d12_shader_runner(r); + + if (runner->r.maximum_shader_model < SHADER_MODEL_4_0) + return false; + + if (runner->r.minimum_shader_model >= SHADER_MODEL_6_0 && !runner->dxc_compiler) + return false; + + return true; +} + #define MAX_RESOURCE_DESCRIPTORS (MAX_RESOURCES * 2)
static struct resource *d3d12_runner_create_resource(struct shader_runner *r, const struct resource_params *params) @@ -567,6 +580,7 @@ static void d3d12_runner_release_readback(struct shader_runner *r, struct resour
static const struct shader_runner_ops d3d12_runner_ops = { + .check_requirements = d3d12_runner_check_requirements, .create_resource = d3d12_runner_create_resource, .destroy_resource = d3d12_runner_destroy_resource, .dispatch = d3d12_runner_dispatch, diff --git a/tests/shader_runner_vulkan.c b/tests/shader_runner_vulkan.c index f89b4d624..ec2d1fc9e 100644 --- a/tests/shader_runner_vulkan.c +++ b/tests/shader_runner_vulkan.c @@ -83,6 +83,18 @@ static struct vulkan_shader_runner *vulkan_shader_runner(struct shader_runner *r
#define VK_CALL(f) (runner->f)
+static bool vulkan_runner_check_requirements(struct shader_runner *r) +{ + struct vulkan_shader_runner *runner = vulkan_shader_runner(r); + + if (runner->r.maximum_shader_model < SHADER_MODEL_4_0) + return false; + if (runner->r.minimum_shader_model >= SHADER_MODEL_6_0) + return false; + + return true; +} + static void begin_command_buffer(struct vulkan_shader_runner *runner) { VkCommandBufferBeginInfo buffer_begin_desc = {.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO}; @@ -1157,6 +1169,7 @@ static void vulkan_runner_release_readback(struct shader_runner *r, struct resou
static const struct shader_runner_ops vulkan_runner_ops = { + .check_requirements = vulkan_runner_check_requirements, .create_resource = vulkan_runner_create_resource, .destroy_resource = vulkan_runner_destroy_resource, .dispatch = vulkan_runner_dispatch,
From: Francisco Casas fcasas@codeweavers.com
In case that the backend selects a shader model that is higher than the minimum in the provided range, it doesn't make sense to check todos(...), fails(...), or notimpl(...) against the minimum.
So the backend must report the selected model for the given range (if any). This is done through the check_requirements() function. --- tests/shader_runner.c | 23 +++++++++++++---------- tests/shader_runner.h | 6 ++++-- tests/shader_runner_d3d11.c | 8 ++++---- tests/shader_runner_d3d12.c | 12 ++++++------ tests/shader_runner_d3d9.c | 6 ++++-- tests/shader_runner_vulkan.c | 8 ++++---- 6 files changed, 35 insertions(+), 28 deletions(-)
diff --git a/tests/shader_runner.c b/tests/shader_runner.c index 77ee63dd0..0705ddc93 100644 --- a/tests/shader_runner.c +++ b/tests/shader_runner.c @@ -499,9 +499,9 @@ static void parse_test_directive(struct shader_runner *runner, const char *line) if (match_string(line, "todo", &line)) runner->is_todo = true; else if (match_string(line, "todo(sm<6)", &line)) - runner->is_todo = runner->minimum_shader_model < SHADER_MODEL_6_0; + runner->is_todo = runner->selected_shader_model < SHADER_MODEL_6_0; else if (match_string(line, "todo(sm>=6)", &line)) - runner->is_todo = runner->minimum_shader_model >= SHADER_MODEL_6_0; + runner->is_todo = runner->selected_shader_model >= SHADER_MODEL_6_0;
if (match_string(line, "dispatch", &line)) { @@ -937,7 +937,7 @@ result_release: static void compile_shader(struct shader_runner *runner, IDxcCompiler3 *dxc_compiler, const char *source, size_t len, enum shader_type type, HRESULT expect) { - bool use_dxcompiler = runner->minimum_shader_model >= SHADER_MODEL_6_0; + bool use_dxcompiler = runner->selected_shader_model >= SHADER_MODEL_6_0; ID3D10Blob *blob = NULL, *errors = NULL; char profile[7]; HRESULT hr; @@ -960,7 +960,7 @@ static void compile_shader(struct shader_runner *runner, IDxcCompiler3 *dxc_comp } else { - sprintf(profile, "%s_%s", shader_type_string(type), shader_models[runner->minimum_shader_model]); + sprintf(profile, "%s_%s", shader_type_string(type), shader_models[runner->selected_shader_model]); hr = D3DCompile(source, len, NULL, NULL, NULL, "main", profile, runner->compile_options, 0, &blob, &errors); } hr = map_unidentified_hrs(hr); @@ -990,7 +990,7 @@ static enum parse_state read_shader_directive(struct shader_runner *runner, enum /* 'todo' is not meaningful when dxcompiler is in use, so it has no '(sm<6) qualifier. */ if (match_directive_substring(src, "todo", &src)) { - if (runner->minimum_shader_model >= SHADER_MODEL_6_0) + if (runner->selected_shader_model >= SHADER_MODEL_6_0) continue; if (state == STATE_SHADER_COMPUTE) state = STATE_SHADER_COMPUTE_TODO; @@ -1005,17 +1005,17 @@ static enum parse_state read_shader_directive(struct shader_runner *runner, enum } else if (match_directive_substring(src, "fail(sm<6)", &src)) { - if (runner->minimum_shader_model < SHADER_MODEL_6_0) + if (runner->selected_shader_model < SHADER_MODEL_6_0) *expect_hr = E_FAIL; } else if (match_directive_substring(src, "fail(sm>=6)", &src)) { - if (runner->minimum_shader_model >= SHADER_MODEL_6_0) + if (runner->selected_shader_model >= SHADER_MODEL_6_0) *expect_hr = E_FAIL; } else if (match_directive_substring(src, "notimpl(sm<6)", &src)) { - if (runner->minimum_shader_model < SHADER_MODEL_6_0) + if (runner->selected_shader_model < SHADER_MODEL_6_0) *expect_hr = E_NOTIMPL; } else @@ -1055,6 +1055,9 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o runner->minimum_shader_model = minimum_shader_model; runner->maximum_shader_model = maximum_shader_model;
+ assert(runner->ops->check_requirements); + skip_tests = !runner->ops->check_requirements(runner, &runner->selected_shader_model); + for (;;) { char *ret = fgets(line_buffer, sizeof(line_buffer), f); @@ -1073,9 +1076,8 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o break;
case STATE_REQUIRE: - assert(runner->ops->check_requirements); if (runner->maximum_shader_model < runner->minimum_shader_model - || !runner->ops->check_requirements(runner)) + || !runner->ops->check_requirements(runner, &runner->selected_shader_model)) { skip_tests = true; } @@ -1218,6 +1220,7 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o state = STATE_REQUIRE; runner->minimum_shader_model = minimum_shader_model; runner->maximum_shader_model = maximum_shader_model; + runner->selected_shader_model = minimum_shader_model; runner->compile_options = 0; skip_tests = false; } diff --git a/tests/shader_runner.h b/tests/shader_runner.h index f6ef44a71..e52a44596 100644 --- a/tests/shader_runner.h +++ b/tests/shader_runner.h @@ -120,6 +120,7 @@ struct shader_runner char *cs_source; enum shader_model minimum_shader_model; enum shader_model maximum_shader_model; + enum shader_model selected_shader_model;
bool last_render_failed;
@@ -140,8 +141,9 @@ struct shader_runner
struct shader_runner_ops { - /* Returns false if unable to run the given tests. */ - bool (*check_requirements)(struct shader_runner *runner); + /* Returns false if unable to run the given tests. Otherwise, it also outputs the lowest model + * supported within the range provided in the runner. */ + bool (*check_requirements)(struct shader_runner *runner, enum shader_model *model);
struct resource *(*create_resource)(struct shader_runner *runner, const struct resource_params *params); void (*destroy_resource)(struct shader_runner *runner, struct resource *resource); diff --git a/tests/shader_runner_d3d11.c b/tests/shader_runner_d3d11.c index 7a8f8dac0..df704da8c 100644 --- a/tests/shader_runner_d3d11.c +++ b/tests/shader_runner_d3d11.c @@ -80,15 +80,13 @@ static ID3D10Blob *compile_shader(const struct d3d11_shader_runner *runner, cons
static const char *const shader_models[] = { - [SHADER_MODEL_2_0] = "4_0", - [SHADER_MODEL_3_0] = "4_0", [SHADER_MODEL_4_0] = "4_0", [SHADER_MODEL_4_1] = "4_1", [SHADER_MODEL_5_0] = "5_0", [SHADER_MODEL_5_1] = "5_1", };
- sprintf(profile, "%s_%s", type, shader_models[runner->r.minimum_shader_model]); + sprintf(profile, "%s_%s", type, shader_models[runner->r.selected_shader_model]); hr = D3DCompile(source, strlen(source), NULL, NULL, NULL, "main", profile, runner->r.compile_options, 0, &blob, &errors); ok(hr == S_OK, "Failed to compile shader, hr %#lx.\n", hr); if (errors) @@ -100,7 +98,7 @@ static ID3D10Blob *compile_shader(const struct d3d11_shader_runner *runner, cons return blob; }
-static bool d3d11_runner_check_requirements(struct shader_runner *r) +static bool d3d11_runner_check_requirements(struct shader_runner *r, enum shader_model *model) { struct d3d11_shader_runner *runner = d3d11_shader_runner(r);
@@ -109,6 +107,8 @@ static bool d3d11_runner_check_requirements(struct shader_runner *r) if (runner->r.minimum_shader_model >= SHADER_MODEL_6_0) return false;
+ *model = max(SHADER_MODEL_4_0, runner->r.minimum_shader_model); + return true; }
diff --git a/tests/shader_runner_d3d12.c b/tests/shader_runner_d3d12.c index c54cb5384..ccac95189 100644 --- a/tests/shader_runner_d3d12.c +++ b/tests/shader_runner_d3d12.c @@ -67,8 +67,6 @@ static ID3D10Blob *compile_shader(const struct d3d12_shader_runner *runner, cons
static const char *const shader_models[] = { - [SHADER_MODEL_2_0] = "4_0", - [SHADER_MODEL_3_0] = "4_0", [SHADER_MODEL_4_0] = "4_0", [SHADER_MODEL_4_1] = "4_1", [SHADER_MODEL_5_0] = "5_0", @@ -76,14 +74,14 @@ static ID3D10Blob *compile_shader(const struct d3d12_shader_runner *runner, cons [SHADER_MODEL_6_0] = "6_0", };
- if (runner->r.minimum_shader_model >= SHADER_MODEL_6_0) + if (runner->r.selected_shader_model >= SHADER_MODEL_6_0) { assert(runner->dxc_compiler); hr = dxc_compiler_compile_shader(runner->dxc_compiler, type, runner->r.compile_options, source, &blob, &errors); } else { - sprintf(profile, "%s_%s", shader_type_string(type), shader_models[runner->r.minimum_shader_model]); + sprintf(profile, "%s_%s", shader_type_string(type), shader_models[runner->r.selected_shader_model]); hr = D3DCompile(source, strlen(source), NULL, NULL, NULL, "main", profile, runner->r.compile_options, 0, &blob, &errors); } ok(FAILED(hr) == !blob, "Got unexpected hr %#x, blob %p.\n", hr, blob); @@ -96,7 +94,7 @@ static ID3D10Blob *compile_shader(const struct d3d12_shader_runner *runner, cons return blob; }
-static bool d3d12_runner_check_requirements(struct shader_runner *r) +static bool d3d12_runner_check_requirements(struct shader_runner *r, enum shader_model *model) { struct d3d12_shader_runner *runner = d3d12_shader_runner(r);
@@ -106,6 +104,8 @@ static bool d3d12_runner_check_requirements(struct shader_runner *r) if (runner->r.minimum_shader_model >= SHADER_MODEL_6_0 && !runner->dxc_compiler) return false;
+ *model = max(SHADER_MODEL_4_0, runner->r.minimum_shader_model); + return true; }
@@ -336,7 +336,7 @@ static bool d3d12_runner_dispatch(struct shader_runner *r, unsigned int x, unsig size_t i;
cs_code = compile_shader(runner, runner->r.cs_source, SHADER_TYPE_CS); - todo_if(runner->r.is_todo && runner->r.minimum_shader_model < SHADER_MODEL_6_0) ok(cs_code, "Failed to compile shader.\n"); + todo_if(runner->r.is_todo && runner->r.selected_shader_model < SHADER_MODEL_6_0) ok(cs_code, "Failed to compile shader.\n"); if (!cs_code) return false;
diff --git a/tests/shader_runner_d3d9.c b/tests/shader_runner_d3d9.c index 0b33c1c53..fbd7d6b5b 100644 --- a/tests/shader_runner_d3d9.c +++ b/tests/shader_runner_d3d9.c @@ -68,7 +68,7 @@ static ID3D10Blob *compile_shader(const struct d3d9_shader_runner *runner, const [SHADER_MODEL_3_0] = "3_0", };
- sprintf(profile, "%s_%s", type, shader_models[runner->r.minimum_shader_model]); + sprintf(profile, "%s_%s", type, shader_models[runner->r.selected_shader_model]); hr = D3DCompile(source, strlen(source), NULL, NULL, NULL, "main", profile, runner->r.compile_options, 0, &blob, &errors); ok(hr == S_OK, "Failed to compile shader, hr %#lx.\n", hr); if (errors) @@ -183,13 +183,15 @@ static D3DTEXTUREADDRESS sampler_address_to_d3d9(D3D12_TEXTURE_ADDRESS_MODE addr vkd3d_unreachable(); }
-static bool d3d9_runner_check_requirements(struct shader_runner *r) +static bool d3d9_runner_check_requirements(struct shader_runner *r, enum shader_model *model) { struct d3d9_shader_runner *runner = d3d9_shader_runner(r);
if (runner->r.minimum_shader_model >= SHADER_MODEL_4_0) return false;
+ *model = runner->r.minimum_shader_model; + return true; }
diff --git a/tests/shader_runner_vulkan.c b/tests/shader_runner_vulkan.c index ec2d1fc9e..131e394b0 100644 --- a/tests/shader_runner_vulkan.c +++ b/tests/shader_runner_vulkan.c @@ -83,7 +83,7 @@ static struct vulkan_shader_runner *vulkan_shader_runner(struct shader_runner *r
#define VK_CALL(f) (runner->f)
-static bool vulkan_runner_check_requirements(struct shader_runner *r) +static bool vulkan_runner_check_requirements(struct shader_runner *r, enum shader_model *model) { struct vulkan_shader_runner *runner = vulkan_shader_runner(r);
@@ -92,6 +92,8 @@ static bool vulkan_runner_check_requirements(struct shader_runner *r) if (runner->r.minimum_shader_model >= SHADER_MODEL_6_0) return false;
+ *model = max(SHADER_MODEL_4_0, runner->r.minimum_shader_model); + return true; }
@@ -407,8 +409,6 @@ static bool compile_shader(const struct vulkan_shader_runner *runner, const char
static const char *const shader_models[] = { - [SHADER_MODEL_2_0] = "4_0", - [SHADER_MODEL_3_0] = "4_0", [SHADER_MODEL_4_0] = "4_0", [SHADER_MODEL_4_1] = "4_1", [SHADER_MODEL_5_0] = "5_0", @@ -451,7 +451,7 @@ static bool compile_shader(const struct vulkan_shader_runner *runner, const char }
hlsl_info.entry_point = "main"; - sprintf(profile, "%s_%s", type, shader_models[runner->r.minimum_shader_model]); + sprintf(profile, "%s_%s", type, shader_models[runner->r.selected_shader_model]); hlsl_info.profile = profile;
ret = vkd3d_shader_compile(&info, dxbc, &messages);
From: Francisco Casas fcasas@codeweavers.com
So, the idea is that running run_shader_tests() means to ask the backend to run the shader_test file so that each test is executed with the lowest profile the backend actually supports (or not excuted at all if there isn't any).
Considering that in the future we may want to test all profiles for all backends, for instance, for Windows:
for(m = SHADER_MODEL_2_0; m <= SHADER_MODEL_6_0; ++m) { run_shader_tests_d3d9(m, m); run_shader_tests_d3d11(m, m) run_shader_tests_d3d12(m, m, dxc_compiler); } --- tests/shader_runner.c | 25 ++++++++++++++----------- tests/shader_runner.h | 10 +++++----- tests/shader_runner_d3d11.c | 4 ++-- tests/shader_runner_d3d12.c | 4 ++-- tests/shader_runner_d3d9.c | 4 ++-- tests/shader_runner_vulkan.c | 4 ++-- 6 files changed, 27 insertions(+), 24 deletions(-)
diff --git a/tests/shader_runner.c b/tests/shader_runner.c index 0705ddc93..f4121a54b 100644 --- a/tests/shader_runner.c +++ b/tests/shader_runner.c @@ -955,7 +955,8 @@ static void compile_shader(struct shader_runner *runner, IDxcCompiler3 *dxc_comp
if (use_dxcompiler) { - assert(dxc_compiler); + if (!dxc_compiler) + return; hr = dxc_compiler_compile_shader(dxc_compiler, type, runner->compile_options, source, &blob, &errors); } else @@ -1030,6 +1031,8 @@ static enum parse_state read_shader_directive(struct shader_runner *runner, enum return state; }
+/* Ask the backend to run the shader_test file so that each test is executed with the lowest profile + * it can support within the range of shader models specified, if any. */ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_ops *ops, void *dxc_compiler, enum shader_model minimum_shader_model, enum shader_model maximum_shader_model) { @@ -1503,13 +1506,13 @@ START_TEST(shader_runner) trace("Running tests from a Windows cross build\n");
trace("Compiling shaders with d3dcompiler_47.dll and executing with d3d9.dll\n"); - run_shader_tests_d3d9(); + run_shader_tests_d3d9(SHADER_MODEL_2_0, SHADER_MODEL_6_0);
trace("Compiling shaders with d3dcompiler_47.dll and executing with d3d11.dll\n"); - run_shader_tests_d3d11(); + run_shader_tests_d3d11(SHADER_MODEL_2_0, SHADER_MODEL_6_0);
trace("Compiling shaders with d3dcompiler_47.dll and executing with d3d12.dll\n"); - run_shader_tests_d3d12(NULL, SHADER_MODEL_4_0, SHADER_MODEL_5_1); + run_shader_tests_d3d12(SHADER_MODEL_2_0, SHADER_MODEL_6_0, NULL);
print_dll_version("d3dcompiler_47.dll"); print_dll_version("dxgi.dll"); @@ -1520,18 +1523,18 @@ START_TEST(shader_runner) trace("Running tests from a Windows non-cross build\n");
trace("Compiling shaders with vkd3d-shader and executing with d3d9.dll\n"); - run_shader_tests_d3d9(); + run_shader_tests_d3d9(SHADER_MODEL_2_0, SHADER_MODEL_6_0);
trace("Compiling shaders with vkd3d-shader and executing with d3d11.dll\n"); - run_shader_tests_d3d11(); + run_shader_tests_d3d11(SHADER_MODEL_2_0, SHADER_MODEL_6_0);
trace("Compiling shaders with vkd3d-shader and executing with vkd3d\n"); - run_shader_tests_d3d12(NULL, SHADER_MODEL_4_0, SHADER_MODEL_5_1); + run_shader_tests_d3d12(SHADER_MODEL_2_0, SHADER_MODEL_6_0, NULL);
if ((dxc_compiler = dxcompiler_create())) { trace("Compiling shaders with dxcompiler and executing with vkd3d\n"); - run_shader_tests_d3d12(dxc_compiler, SHADER_MODEL_6_0, SHADER_MODEL_6_0); + run_shader_tests_d3d12(SHADER_MODEL_6_0, SHADER_MODEL_6_0, dxc_compiler); IDxcCompiler3_Release(dxc_compiler); print_dll_version(SONAME_LIBDXCOMPILER); } @@ -1542,15 +1545,15 @@ START_TEST(shader_runner) trace("Running tests from a Unix build\n");
trace("Compiling shaders with vkd3d-shader and executing with Vulkan\n"); - run_shader_tests_vulkan(); + run_shader_tests_vulkan(SHADER_MODEL_2_0, SHADER_MODEL_6_0);
trace("Compiling shaders with vkd3d-shader and executing with vkd3d\n"); - run_shader_tests_d3d12(NULL, SHADER_MODEL_4_0, SHADER_MODEL_5_1); + run_shader_tests_d3d12(SHADER_MODEL_2_0, SHADER_MODEL_6_0, NULL);
if ((dxc_compiler = dxcompiler_create())) { trace("Compiling shaders with dxcompiler and executing with vkd3d\n"); - run_shader_tests_d3d12(dxc_compiler, SHADER_MODEL_6_0, SHADER_MODEL_6_0); + run_shader_tests_d3d12(SHADER_MODEL_6_0, SHADER_MODEL_6_0, dxc_compiler); IDxcCompiler3_Release(dxc_compiler); } #endif diff --git a/tests/shader_runner.h b/tests/shader_runner.h index e52a44596..b91e23d08 100644 --- a/tests/shader_runner.h +++ b/tests/shader_runner.h @@ -169,10 +169,10 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o enum shader_model minimum_shader_model, enum shader_model maximum_shader_model);
#ifdef _WIN32 -void run_shader_tests_d3d9(void); -void run_shader_tests_d3d11(void); +void run_shader_tests_d3d9(enum shader_model minimum_shader_model, enum shader_model maximum_shader_model); +void run_shader_tests_d3d11(enum shader_model minimum_shader_model, enum shader_model maximum_shader_model); #else -void run_shader_tests_vulkan(void); +void run_shader_tests_vulkan(enum shader_model minimum_shader_model, enum shader_model maximum_shader_model); #endif -void run_shader_tests_d3d12(void *dxc_compiler, enum shader_model minimum_shader_model, - enum shader_model maximum_shader_model); +void run_shader_tests_d3d12(enum shader_model minimum_shader_model, enum shader_model maximum_shader_model, + void *dxc_compiler); diff --git a/tests/shader_runner_d3d11.c b/tests/shader_runner_d3d11.c index df704da8c..978dd471e 100644 --- a/tests/shader_runner_d3d11.c +++ b/tests/shader_runner_d3d11.c @@ -745,7 +745,7 @@ static const struct shader_runner_ops d3d11_runner_ops = .release_readback = d3d11_runner_release_readback, };
-void run_shader_tests_d3d11(void) +void run_shader_tests_d3d11(enum shader_model minimum_shader_model, enum shader_model maximum_shader_model) { struct d3d11_shader_runner runner; HMODULE dxgi_module, d3d11_module; @@ -760,7 +760,7 @@ void run_shader_tests_d3d11(void) init_adapter_info(); if (init_test_context(&runner)) { - run_shader_tests(&runner.r, &d3d11_runner_ops, NULL, SHADER_MODEL_2_0, SHADER_MODEL_5_1); + run_shader_tests(&runner.r, &d3d11_runner_ops, NULL, minimum_shader_model, maximum_shader_model); destroy_test_context(&runner); } } diff --git a/tests/shader_runner_d3d12.c b/tests/shader_runner_d3d12.c index ccac95189..98e3e7c0f 100644 --- a/tests/shader_runner_d3d12.c +++ b/tests/shader_runner_d3d12.c @@ -589,8 +589,8 @@ static const struct shader_runner_ops d3d12_runner_ops = .release_readback = d3d12_runner_release_readback, };
-void run_shader_tests_d3d12(void *dxc_compiler, enum shader_model minimum_shader_model, - enum shader_model maximum_shader_model) +void run_shader_tests_d3d12(enum shader_model minimum_shader_model, enum shader_model maximum_shader_model, + void *dxc_compiler) { static const struct test_context_desc desc = { diff --git a/tests/shader_runner_d3d9.c b/tests/shader_runner_d3d9.c index fbd7d6b5b..b05d178b2 100644 --- a/tests/shader_runner_d3d9.c +++ b/tests/shader_runner_d3d9.c @@ -531,7 +531,7 @@ static const struct shader_runner_ops d3d9_runner_ops = .release_readback = d3d9_runner_release_readback, };
-void run_shader_tests_d3d9(void) +void run_shader_tests_d3d9(enum shader_model minimum_shader_model, enum shader_model maximum_shader_model) { struct d3d9_shader_runner runner; HMODULE d3d9_module; @@ -543,7 +543,7 @@ void run_shader_tests_d3d9(void)
init_adapter_info(); init_test_context(&runner); - run_shader_tests(&runner.r, &d3d9_runner_ops, NULL, SHADER_MODEL_2_0, SHADER_MODEL_3_0); + run_shader_tests(&runner.r, &d3d9_runner_ops, NULL, minimum_shader_model, maximum_shader_model); destroy_test_context(&runner); } FreeLibrary(d3d9_module); diff --git a/tests/shader_runner_vulkan.c b/tests/shader_runner_vulkan.c index 131e394b0..418f3e3bd 100644 --- a/tests/shader_runner_vulkan.c +++ b/tests/shader_runner_vulkan.c @@ -1390,14 +1390,14 @@ static void cleanup_vulkan_runner(struct vulkan_shader_runner *runner) VK_CALL(vkDestroyInstance(runner->instance, NULL)); }
-void run_shader_tests_vulkan(void) +void run_shader_tests_vulkan(enum shader_model minimum_shader_model, enum shader_model maximum_shader_model) { struct vulkan_shader_runner runner = {0};
if (!init_vulkan_runner(&runner)) return;
- run_shader_tests(&runner.r, &vulkan_runner_ops, NULL, SHADER_MODEL_2_0, SHADER_MODEL_5_1); + run_shader_tests(&runner.r, &vulkan_runner_ops, NULL, minimum_shader_model, maximum_shader_model);
cleanup_vulkan_runner(&runner); }
From: Francisco Casas fcasas@codeweavers.com
This would allow us to test compilation of SM1 shaders even if we don't have an available backend that can run them. --- tests/shader_runner.c | 65 ++++++++++++++++++++++++++++++------ tests/shader_runner.h | 2 +- tests/shader_runner_d3d11.c | 2 +- tests/shader_runner_d3d12.c | 2 +- tests/shader_runner_d3d9.c | 2 +- tests/shader_runner_vulkan.c | 2 +- 6 files changed, 60 insertions(+), 15 deletions(-)
diff --git a/tests/shader_runner.c b/tests/shader_runner.c index f4121a54b..2a5a9ec71 100644 --- a/tests/shader_runner.c +++ b/tests/shader_runner.c @@ -1032,9 +1032,11 @@ static enum parse_state read_shader_directive(struct shader_runner *runner, enum }
/* Ask the backend to run the shader_test file so that each test is executed with the lowest profile - * it can support within the range of shader models specified, if any. */ + * it can support within the range of shader models specified, if any. + * If check_syntax is true, then the shaders are compiled on the [shader] directives using the + * available compiler. If ops is NULL, execution tests are skipped. */ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_ops *ops, void *dxc_compiler, - enum shader_model minimum_shader_model, enum shader_model maximum_shader_model) + enum shader_model minimum_shader_model, enum shader_model maximum_shader_model, bool check_syntax) { size_t shader_source_size = 0, shader_source_len = 0; struct resource_params current_resource; @@ -1058,8 +1060,16 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o runner->minimum_shader_model = minimum_shader_model; runner->maximum_shader_model = maximum_shader_model;
- assert(runner->ops->check_requirements); - skip_tests = !runner->ops->check_requirements(runner, &runner->selected_shader_model); + if (runner->ops) + { + skip_tests = !runner->ops->check_requirements(runner, &runner->selected_shader_model); + } + else + { + /* TODO: Make runner->selected_shader_model = runner->minimum_shader_model so that + * check_syntax is allowed to compile SM1 profiles. */ + runner->selected_shader_model = max(SHADER_MODEL_4_0, runner->minimum_shader_model); + }
for (;;) { @@ -1079,11 +1089,18 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o break;
case STATE_REQUIRE: + assert(!runner->ops || runner->ops->check_requirements); if (runner->maximum_shader_model < runner->minimum_shader_model - || !runner->ops->check_requirements(runner, &runner->selected_shader_model)) + || (runner->ops && !runner->ops->check_requirements(runner, &runner->selected_shader_model))) { skip_tests = true; } + if (!runner->ops) + { + /* TODO: Make runner->selected_shader_model = runner->minimum_shader_model so that + * check_syntax is allowed to compile SM1 profiles. */ + runner->selected_shader_model = max(SHADER_MODEL_4_0, runner->minimum_shader_model); + } break;
case STATE_RESOURCE: @@ -1092,14 +1109,15 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o * textures with data type other than float). */ if (!skip_tests) { - set_resource(runner, runner->ops->create_resource(runner, ¤t_resource)); + if (runner->ops) + set_resource(runner, runner->ops->create_resource(runner, ¤t_resource)); } free(current_resource.data); break;
case STATE_SHADER_COMPUTE: case STATE_SHADER_COMPUTE_TODO: - if (!skip_tests) + if (check_syntax && !skip_tests) { todo_if (state == STATE_SHADER_COMPUTE_TODO) compile_shader(runner, dxc_compiler, shader_source, shader_source_len, SHADER_TYPE_CS, @@ -1114,7 +1132,7 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o
case STATE_SHADER_PIXEL: case STATE_SHADER_PIXEL_TODO: - if (!skip_tests) + if (check_syntax && !skip_tests) { todo_if (state == STATE_SHADER_PIXEL_TODO) compile_shader(runner, dxc_compiler, shader_source, shader_source_len, SHADER_TYPE_PS, @@ -1129,7 +1147,7 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o
case STATE_SHADER_VERTEX: case STATE_SHADER_VERTEX_TODO: - if (!skip_tests) + if (check_syntax && !skip_tests) { todo_if (state == STATE_SHADER_VERTEX_TODO) compile_shader(runner, dxc_compiler, shader_source, shader_source_len, SHADER_TYPE_VS, @@ -1387,7 +1405,10 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o /* Compilation which fails with dxcompiler is not 'todo', therefore the tests are * not 'todo' either. They cannot run, so skip them entirely. */ if (!skip_tests && SUCCEEDED(expect_hr)) - parse_test_directive(runner, line); + { + if (runner->ops) + parse_test_directive(runner, line); + } break; } } @@ -1400,6 +1421,7 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o free(runner->ps_source); for (i = 0; i < runner->resource_count; ++i) { + assert(runner->ops); if (runner->resources[i]) runner->ops->destroy_resource(runner, runner->resources[i]); } @@ -1407,6 +1429,14 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o fclose(f); }
+static void run_shader_compilation_tests(enum shader_model minimum_shader_model, + enum shader_model maximum_shader_model, void *dxc_compiler) +{ + struct shader_runner runner = {}; + + return run_shader_tests(&runner, NULL, dxc_compiler, minimum_shader_model, maximum_shader_model, true); +} + #ifdef _WIN32 static void print_dll_version(const char *file_name) { @@ -1505,6 +1535,9 @@ START_TEST(shader_runner) #if defined(VKD3D_CROSSTEST) trace("Running tests from a Windows cross build\n");
+ trace("Compiling shaders with d3dcompiler_47.dll\n"); + run_shader_compilation_tests(SHADER_MODEL_2_0, SHADER_MODEL_6_0, NULL); + trace("Compiling shaders with d3dcompiler_47.dll and executing with d3d9.dll\n"); run_shader_tests_d3d9(SHADER_MODEL_2_0, SHADER_MODEL_6_0);
@@ -1522,6 +1555,9 @@ START_TEST(shader_runner) #elif defined(_WIN32) trace("Running tests from a Windows non-cross build\n");
+ trace("Compiling shaders with vkd3d-shader\n"); + run_shader_compilation_tests(SHADER_MODEL_2_0, SHADER_MODEL_6_0, NULL); + trace("Compiling shaders with vkd3d-shader and executing with d3d9.dll\n"); run_shader_tests_d3d9(SHADER_MODEL_2_0, SHADER_MODEL_6_0);
@@ -1533,6 +1569,9 @@ START_TEST(shader_runner)
if ((dxc_compiler = dxcompiler_create())) { + trace("Compiling shaders with dxcompiler\n"); + run_shader_compilation_tests(SHADER_MODEL_6_0, SHADER_MODEL_6_0, dxc_compiler); + trace("Compiling shaders with dxcompiler and executing with vkd3d\n"); run_shader_tests_d3d12(SHADER_MODEL_6_0, SHADER_MODEL_6_0, dxc_compiler); IDxcCompiler3_Release(dxc_compiler); @@ -1544,6 +1583,9 @@ START_TEST(shader_runner) #else trace("Running tests from a Unix build\n");
+ trace("Compiling shaders with vkd3d-shader\n"); + run_shader_compilation_tests(SHADER_MODEL_2_0, SHADER_MODEL_6_0, NULL); + trace("Compiling shaders with vkd3d-shader and executing with Vulkan\n"); run_shader_tests_vulkan(SHADER_MODEL_2_0, SHADER_MODEL_6_0);
@@ -1552,6 +1594,9 @@ START_TEST(shader_runner)
if ((dxc_compiler = dxcompiler_create())) { + trace("Compiling shaders with dxcompiler\n"); + run_shader_compilation_tests(SHADER_MODEL_6_0, SHADER_MODEL_6_0, dxc_compiler); + trace("Compiling shaders with dxcompiler and executing with vkd3d\n"); run_shader_tests_d3d12(SHADER_MODEL_6_0, SHADER_MODEL_6_0, dxc_compiler); IDxcCompiler3_Release(dxc_compiler); diff --git a/tests/shader_runner.h b/tests/shader_runner.h index b91e23d08..9763b3661 100644 --- a/tests/shader_runner.h +++ b/tests/shader_runner.h @@ -166,7 +166,7 @@ HRESULT dxc_compiler_compile_shader(void *dxc_compiler, enum shader_type type, u const char *hlsl, ID3D10Blob **blob_out, ID3D10Blob **errors_out);
void run_shader_tests(struct shader_runner *runner, const struct shader_runner_ops *ops, void *dxc_compiler, - enum shader_model minimum_shader_model, enum shader_model maximum_shader_model); + enum shader_model minimum_shader_model, enum shader_model maximum_shader_model, bool check_syntax);
#ifdef _WIN32 void run_shader_tests_d3d9(enum shader_model minimum_shader_model, enum shader_model maximum_shader_model); diff --git a/tests/shader_runner_d3d11.c b/tests/shader_runner_d3d11.c index 978dd471e..ab7eb94fa 100644 --- a/tests/shader_runner_d3d11.c +++ b/tests/shader_runner_d3d11.c @@ -760,7 +760,7 @@ void run_shader_tests_d3d11(enum shader_model minimum_shader_model, enum shader_ init_adapter_info(); if (init_test_context(&runner)) { - run_shader_tests(&runner.r, &d3d11_runner_ops, NULL, minimum_shader_model, maximum_shader_model); + run_shader_tests(&runner.r, &d3d11_runner_ops, NULL, minimum_shader_model, maximum_shader_model, false); destroy_test_context(&runner); } } diff --git a/tests/shader_runner_d3d12.c b/tests/shader_runner_d3d12.c index 98e3e7c0f..8f15f1bf8 100644 --- a/tests/shader_runner_d3d12.c +++ b/tests/shader_runner_d3d12.c @@ -622,7 +622,7 @@ void run_shader_tests_d3d12(enum shader_model minimum_shader_model, enum shader_ runner.compute_allocator, NULL, &IID_ID3D12GraphicsCommandList, (void **)&runner.compute_list); ok(hr == S_OK, "Failed to create command list, hr %#x.\n", hr);
- run_shader_tests(&runner.r, &d3d12_runner_ops, dxc_compiler, minimum_shader_model, maximum_shader_model); + run_shader_tests(&runner.r, &d3d12_runner_ops, dxc_compiler, minimum_shader_model, maximum_shader_model, false);
ID3D12GraphicsCommandList_Release(runner.compute_list); ID3D12CommandAllocator_Release(runner.compute_allocator); diff --git a/tests/shader_runner_d3d9.c b/tests/shader_runner_d3d9.c index b05d178b2..3c81f70d5 100644 --- a/tests/shader_runner_d3d9.c +++ b/tests/shader_runner_d3d9.c @@ -543,7 +543,7 @@ void run_shader_tests_d3d9(enum shader_model minimum_shader_model, enum shader_m
init_adapter_info(); init_test_context(&runner); - run_shader_tests(&runner.r, &d3d9_runner_ops, NULL, minimum_shader_model, maximum_shader_model); + run_shader_tests(&runner.r, &d3d9_runner_ops, NULL, minimum_shader_model, maximum_shader_model, false); destroy_test_context(&runner); } FreeLibrary(d3d9_module); diff --git a/tests/shader_runner_vulkan.c b/tests/shader_runner_vulkan.c index 418f3e3bd..1c333e82d 100644 --- a/tests/shader_runner_vulkan.c +++ b/tests/shader_runner_vulkan.c @@ -1397,7 +1397,7 @@ void run_shader_tests_vulkan(enum shader_model minimum_shader_model, enum shader if (!init_vulkan_runner(&runner)) return;
- run_shader_tests(&runner.r, &vulkan_runner_ops, NULL, minimum_shader_model, maximum_shader_model); + run_shader_tests(&runner.r, &vulkan_runner_ops, NULL, minimum_shader_model, maximum_shader_model, false);
cleanup_vulkan_runner(&runner); }
Hi, I haven't reviewed in detail yet, because my feeling is that right now we're adding features to the shader runner without a clear idea of what it should eventually look like, so I'd like to have a discussion about that first.
My personal idea is that eventually all the components in the shader runner pipeline should be as configurable as possible. I had already written something in the [vkd3d-todo](https://wiki.winehq.org/Vkd3d-todo), but let me repeat that here. We basically have the following variables: * the library used to compile the shaders: vkd3d-shader, d3dcompiler_xx.dll or dxcompiler.dll (in particular dxcompiler.dll would have no special treatment as it has now); * the Shader Model to target; * the API to use to run the shader (d3d9, d3d10, d3d11, d3d12, Vulkan, or even none if we just want to test the compiler); * when applicable, whether to execute the shaders using the native implementation or vkd3d; * possibly, one day, the library to use to recompile the shaders (it currently only makes sense on macOS once we support the Apple shader compiler for Metal; in all the other cases the choice is forced).
Ideally there would be a function in the shader runner that takes these options and runs a .shader_test file with that configuration. The shader runner executable would then accept command line options that describe the desired configuration, or run a number of sensible configurations depending on which are available (e.g., whether this is a Linux or MinGW build: clearly native libraries aren't available on Linux, with the exception of dxcompiler.dll). This latest mode would be what would be run on the CI.
By multiplying the number of available configurations the language to define which outcomes are expected might become somewhat more complicated. The usual convention should remain in force: the native implementations are assumed to be the "ground truth" on which `fail` and `notimpl` (and similar ones, if needed) are gauged, while `todo` is used to measure the difference of any other configuration with respect to native. It might at some point turn out to be necessary to evaluate `todo` on something more complicated than just the Shader Model, which is just one of the variables above, but I hope this complexity to be manageable.
This is more of a side thing, but if we eventually come to the point I'm describing the MinGW tests will likely be strictly more powerful than the crosstests, so the crosstests could be dropper. At least for the shader runner, but probably similar considerations could be done for the other crosstests.
What's your view on that? I guess @zfigura, @Mystral and @hverbeet might have something to say too.
On Fri Oct 20 16:14:23 2023 +0000, Giovanni Mascellani wrote:
Hi, I haven't reviewed in detail yet, because my feeling is that right now we're adding features to the shader runner without a clear idea of what it should eventually look like, so I'd like to have a discussion about that first. My personal idea is that eventually all the components in the shader runner pipeline should be as configurable as possible. I had already written something in the [vkd3d-todo](https://wiki.winehq.org/Vkd3d-todo), but let me repeat that here. We basically have the following variables:
- the library used to compile the shaders: vkd3d-shader,
d3dcompiler_xx.dll or dxcompiler.dll (in particular dxcompiler.dll would have no special treatment as it has now);
- the Shader Model to target;
- the API to use to run the shader (d3d9, d3d10, d3d11, d3d12, Vulkan,
or even none if we just want to test the compiler);
- when applicable, whether to execute the shaders using the native
implementation or vkd3d;
- possibly, one day, the library to use to recompile the shaders (it
currently only makes sense on macOS once we support the Apple shader compiler for Metal; in all the other cases the choice is forced). Ideally there would be a function in the shader runner that takes these options and runs a .shader_test file with that configuration. The shader runner executable would then accept command line options that describe the desired configuration, or run a number of sensible configurations depending on which are available (e.g., whether this is a Linux or MinGW build: clearly native libraries aren't available on Linux, with the exception of dxcompiler.dll). This latest mode would be what would be run on the CI. By multiplying the number of available configurations the language to define which outcomes are expected might become somewhat more complicated. The usual convention should remain in force: the native implementations are assumed to be the "ground truth" on which `fail` and `notimpl` (and similar ones, if needed) are gauged, while `todo` is used to measure the difference of any other configuration with respect to native. It might at some point turn out to be necessary to evaluate `todo` on something more complicated than just the Shader Model, which is just one of the variables above, but I hope this complexity to be manageable. This is more of a side thing, but if we eventually come to the point I'm describing the MinGW tests will likely be strictly more powerful than the crosstests, so the crosstests could be dropper. At least for the shader runner, but probably similar considerations could be done for the other crosstests. What's your view on that? I guess @zfigura, @Mystral and @hverbeet might have something to say too.
Hi, I think that this MR helps in this general direction.
I am writing my opinion on the components you mentioned, and explain why these patches (and the following ones) help separating/configuring some of them.
- the Shader Model to target;
I think that here we mean to target a **range** of shader models instead of a specific one.
Currently we have the policy of only running the minimum shader model that intersects the ones that the shader supports, and the range given by the [require] directives. Which I think is a good thing in comparison to running the tests with all the shader models in that intersection (which would be costly).
However, the current implementation doesn't give us the capacity of choosing to run all the shader models, or something more practical like running the lowest SM1 shader model, the lowest SM4 shader model, and the lowest SM6 shader model (if there was a backend that supports them all).
3/4 intents to reinterpret the `minimum_shader_model` and `maximum_shader_model` arguments as "The range of shader models for which we are interested on running a test on this call." instead of "The range of shader models this runner supports.". This so we can call run_shader_tests() 3 times to achieve this example, with this hypothetical backend: ``` run_shader_tests(runner, SHADER_MODEL_2_0, SHADER_MODEL_3_0) run_shader_tests(runner, SHADER_MODEL_4_0, SHADER_MODEL_5_1) run_shader_tests(runner, SHADER_MODEL_6_0, SHADER_MODEL_6_0) ```
Or even running all the profiles: ``` for(m = SHADER_MODEL_2_0; m <= SHADER_MODEL_6_0; ++m) run_shader_tests(runner, m, m) ```
So for each call, the backend runs every test using the lowest shader model it supports, within the provided range, that matches the [require] directive requirements. If there isn't any shader model, it just skips them.
- the API to use to run the shader (d3d9, d3d10, d3d11, d3d12, Vulkan, or even none if we just want to test the compiler);
4/4 adds a `check_syntax` that controls whether the [shader] directives are run, and supports passing a NULL `runner->ops` in which case the other directives are not run, so this way compilation can be tested separately from backend execution (which also requires compilation but on the `draw quad`s on [test] directives).
The reinterpretation of the `minimum_shader_model` and `maximum_shader_model` arguments also helps in this regard, because we could now test compilation on the `minimum_shader_model` and not always start from shader model 4 as we do now, which I do in this patch on part 2:
[63e404fecdea34cc6f1bb32f014b9f7343aeb3cd](https://gitlab.winehq.org/fcasas/vkd3d/-/commit/63e404fecdea34cc6f1bb32f014b...)
- the library used to compile the shaders: vkd3d-shader, d3dcompiler_xx.dll or dxcompiler.dll (in particular dxcompiler.dll would have no special treatment as it has now);
I imagine that in the future we could expect a `struct shader_compiler` (with its `compiler->ops`) as an additional argument to specify the compilation strategies, in case several compilers are available. Unlike the `runner->ops` this one would not be optional, because we would be decoupling the compilation code for each backend.
Ideally there would be a function in the shader runner that takes these options and runs a .shader_test file with that configuration. The shader runner executable would then accept command line options that describe the desired configuration, or run a number of sensible configurations depending on which are available (e.g., whether this is a Linux or MinGW build: clearly native libraries aren't available on Linux, with the exception of dxcompiler.dll). This latest mode would be what would be run on the CI.
Yep, I agree that this would be ideal.
By multiplying the number of available configurations the language to define which outcomes are expected might become somewhat more complicated. The usual convention should remain in force: the native implementations are assumed to be the "ground truth" on which `fail` and `notimpl` (and similar ones, if needed) are gauged, while `todo` is used to measure the difference of any other configuration with respect to native. It might at some point turn out to be necessary to evaluate `todo` on something more complicated than just the Shader Model, which is just one of the variables above, but I hope this complexity to be manageable.
The following patches in my master6i branch (for the second part of this series) allow for more expressive qualifiers, even AND (e.g. `todo(sm>=4,sm<6)`) and OR (e.g. `todo(sm<4) todo(sm>=6)`) expressions.
[ff960463c78be983adbcf805cfd722b40002a214](https://gitlab.winehq.org/fcasas/vkd3d/-/commit/ff960463c78be983adbcf805cfd7...)
[e767803dffb8ab565c9e246b091881f6e91c6f86](https://gitlab.winehq.org/fcasas/vkd3d/-/commit/e767803dffb8ab565c9e246b0918...)
[3a7a316fe6bf8c188c4ced0faa32ce898c222009](https://gitlab.winehq.org/fcasas/vkd3d/-/commit/3a7a316fe6bf8c188c4ced0faa32...)
Maybe whether to ignore or not these qualifiers should be specified in the `struct shader_runner` (and the hypthetical `struct shader_compiler`). For instance dxcompiler.dll should ignore all todo() qualifiers, as it does now.
This is more of a side thing, but if we eventually come to the point I'm describing the MinGW tests will likely be strictly more powerful than the crosstests, so the crosstests could be dropper. At least for the shader runner, but probably similar considerations could be done for the other crosstests.
I like the ability to quickly compile the shader runner on my machine and passing it, with the tests, to the minitestbot though.
Ideally there would be a function in the shader runner that takes these options and runs a .shader_test file with that configuration. The shader runner executable would then accept command line options that describe the desired configuration, or run a number of sensible configurations depending on which are available (e.g., whether this is a Linux or MinGW build: clearly native libraries aren't available on Linux, with the exception of dxcompiler.dll). This latest mode would be what would be run on the CI.
I suppose it's somewhat orthogonal, but what's the advantage to using an external driver for that, instead of just running every desired configuration within the shader_runner binary?
By multiplying the number of available configurations the language to define which outcomes are expected might become somewhat more complicated. The usual convention should remain in force: the native implementations are assumed to be the "ground truth" on which `fail` and `notimpl` (and similar ones, if needed) are gauged, while `todo` is used to measure the difference of any other configuration with respect to native. It might at some point turn out to be necessary to evaluate `todo` on something more complicated than just the Shader Model, which is just one of the variables above, but I hope this complexity to be manageable.
The awkward thing is that currently "todo" means "not implemented in v-s's HLSL compiler". The sm4 compiler has always been complete enough to run tests, so we haven't needed todos for it.
This is changed with the sm6 support. Ideally that's temporary, and eventually "todo" can go back to only ever meaning "not implemented in v-s's HLSL compiler".
I don't think I understand the point of 3/4?
Wrt 4/4, I think I'm pretty close to being able to just hook up sm1 compilation for the Vulkan backend, so that'll end up being a non-issue. Regardless I don't dislike the patch, though.
I don't think I understand the point of 3/4?
Basically what I replied to gio. Reinterpreting the `minimum_shader_model` and `maximum_shader_model` arguments as "The range of shader models for which we are interested on running tests on this call." instead of "The range of shader models this runner supports.", so we can control how many tests we want to run for a given runner (like one for the sm2 to sm3 range and one for the sm4 to the sm5.1 range, or one for every shader model).
If we were to have a backend that can run from SM2 to SM5, we probably wouldn't want to just test SM2 (and only higher profiles when the [require] directive is in place), but rather, we would want to run it once for SM2-3 and once for SM4-5.1. The same for a compiler without `runner->ops`.
So we have to make the distinction between "the lowest SM we are interested in running", and "the lowest SM the backend supports with the given requirements", which becomes more evident in following patches such as [1d07c851b962e9b02cdf07ff68a9ebdfb7d93357](https://gitlab.winehq.org/fcasas/vkd3d/-/commit/1d07c851b962e9b02cdf07ff68a9...).
That is why these minimum_shader and maximum_shader should be passed as arguments to the run_shader_tests_*() functions.
Basically what I replied to gio. Reinterpreting the `minimum_shader_model` and `maximum_shader_model` arguments as "The range of shader models for which we are interested on running tests on this call." instead of "The range of shader models this runner supports.", so we can control how many tests we want to run for a given runner (like one for the sm2 to sm3 range and one for the sm4 to the sm5.1 range, or one for every shader model).
Reinterpreting those arguments makes sense and I definitely agree we should fix that, but I don't understand why 3/4 is doing what it is doing. I would expect the patch to just stop passing those as parameters to run_shader_tests() at all, and instead just initialize those fields in run_shader_tests() itself. Or do that, but also keep the current arguments and instead initialize selected_shader_model from them.
On Sat Oct 21 00:39:08 2023 +0000, Zebediah Figura wrote:
Basically what I replied to gio. Reinterpreting the
`minimum_shader_model` and `maximum_shader_model` arguments as "The range of shader models for which we are interested on running tests on this call." instead of "The range of shader models this runner supports.", so we can control how many tests we want to run for a given runner (like one for the sm2 to sm3 range and one for the sm4 to the sm5.1 range, or one for every shader model). Reinterpreting those arguments makes sense and I definitely agree we should fix that, but I don't understand why 3/4 is doing what it is doing. I would expect the patch to just stop passing those as parameters to run_shader_tests() at all, and instead just initialize those fields in run_shader_tests() itself. Or do that, but also keep the current arguments and instead initialize selected_shader_model from them.
If I understand correctly, you mean that is okay that every run_shader_tests_*() function gets them as parameters, and use them internally to initialize the fields in the runner, so run_shader_tests() itself should not receive them as parameters.
I think nothing preempt us from doing that, except that run_shader_tests() wouldn't be able to determine the selected_shader_model on its own on a run with `ops = NULL` and `check_syntax = true`.
But yep, maybe for consistency even compile-only runs should have their ops defined, even if they are mostly empty functions or NULL function pointers. I will do that.
We had a discussion about this in person and these are the main takeaways I gathered: * We want `.shader_test` file to only have at most one `[require]` directive, and have it at the top of the file, so that we don't have all the problems connected with changing the requirements mid-file. Files that require testing under different requirements will have to be split. * This `[require]` directive should list all the shader models that make sense for whatever is tested, so that there is no need for guesswork (in the form of the current "pick the minimum supported SM" rule) by the runner. For the SMs not supported by the runner, there is nothing to test. This is considered a compromise between running at all possible shader models (expensive) and resorting to complicated logics to pick the shader models that make sense by intersecting what the shader test supports, what the shader runner supports and using some heuristic.
Does everybody feel represented by this? Did I miss anything? We can keep the conversation going.
We want `.shader_test` file to only have at most one `[require]` directive, and have it at the top of the file, so that we don't have all the problems connected with changing the requirements mid-file. Files that require testing under different requirements will have to be split.
Yep, I agree that this is a reasonable way of splitting the tests, even if it costs some redundancy. But I think that that change should be a separate MR.
This `[require]` directive should list all the shader models that make sense for whatever is tested, so that there is no need for guesswork (in the form of the current "pick the minimum supported SM" rule) by the runner. For the SMs not supported by the runner, there is nothing to test. This is considered a compromise between running at all possible shader models (expensive) and resorting to complicated logics to pick the shader models that make sense by intersecting what the shader test supports, what the shader runner supports and using some heuristic.
Yes. Still, @zfigura pointed out that we may still want to make the distinction between the minimum_shader_model and the selected_shader_model. If I understand correctly, this refers to patch 2/4. But I think we can reevaluate if we want to keep it once I send a new version of this MR, because they will be the same, at least in our use cases, once we start hardcoding the shader model ranges.
Hardcoding the shader model ranges is something that we talked about and it is not on this list, but I think it simply consists on making each runner run one test in the SM 2 to SM 3 range, one in the SM 4 to SM 5.1 range, and the SM 6, so runners would either support or not running the whole range and never, start supporting it from the middle, which simplifies some of the logic.
Yes. Still, @zfigura pointed out that we may still want to make the distinction between the minimum_shader_model and the selected_shader_model. If I understand correctly, this refers to patch 2/4. But I think we can reevaluate if we want to keep it once I send a new version of this MR, because they will be the same, at least in our use cases, once we start hardcoding the shader model ranges.
I think I am a bit confused by the fact that we have a few of different `{minimum,maxiumum}_shader_model` around. In 3/4, in particular, there are a pair of them given by the `[require]` directives and another pair given by the runner.
The proposal I am framing is that the `[require]` directive shouldn't really specify a range, but a list of specific SMs, like this: ``` [require] shader model 2.0 shader model 4.0 shader model 5.1 shader model 6.0 ```
The problem with expressing a range, like we do now, is that you either then test only one of the SMs in the range (potentially skipping others that still make sense to test, like if, for instance, there is some behavior difference betweem 4.0, 5.0 and 5.1) or you test all of them, which might be a waste of time. Here, instead, the shader test author can choose all the alternatives that make sense.
Of course, of that list, only the SMs that belong to the range supported by the runner will be tested.
Hardcoding the shader model ranges is something that we talked about and it is not on this list, but I think it simply consists on making each runner run one test in the SM 2 to SM 3 range, one in the SM 4 to SM 5.1 range, and the SM 6,
Yes, something like that. But with the option to choose, for instance, more than one between 4.0 and 5.1 if that test touches stuff that changed between those.
so runners would either support or not running the whole range and never, start supporting it from the middle, which simplifies some of the logic.
I'm not sure I understand what you mean here.
But yep, maybe for consistency even compile-only runs should have their ops defined, even if they are mostly empty functions or NULL function pointers. I will do that.
Yeah, I'd rather have a "null executor" to abstract compilation only, rather than have another flag.
On Wed Oct 25 14:23:55 2023 +0000, Giovanni Mascellani wrote:
But yep, maybe for consistency even compile-only runs should have
their ops defined, even if they are mostly empty functions or NULL function pointers. I will do that. Yeah, I'd rather have a "null executor" to abstract compilation only, rather than have another flag.
Well, after trying to implement that, I think I prefer the check_syntax flag now, because there are many shader runner ops that must be either all defined or all NULL to make sense (specially all those required by parse_test_directive(), and the create_resource() and destroy_resource() pairs), unless we add a lot of checks or a lot of blank functions, which I think would be better if we do it in a separate `shader_runner_only_compilation.c` file.
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Zebediah Figura (＠zfigura)