[PATCH 0/4] MR473: tests/shader-runner: Run SM1 compilation in the shader runner, part 1.
Like the previous MRs, this has the objective of: * Preparing the way for SM1-specific tests. * Running SM1 compilation tests, even if we don't have a runner available to execute them (yet). * Allowing different runners to run through the shader_test file several times, targeting different shader model ranges. The current plan is: * We won't separate compilation tests from execution tests as in !418. * Instead of naming individual shader models in the shader_test files as in !434, we will keep working with the shader model ranges provided in the [require] directives. * We will test SM1 compilation going through the shader_test file twice with the vulkan runner, one for SM1 models and other for SM4 models, and for now we will just give a "todo" for execution tests until they are implemented (@zfigura patches). This first part only includes facilities to properly mark SM1-specific tests (as required by !458) and some refactoring to better work with the idea of intersecting shader model ranges. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473
From: Francisco Casas <fcasas(a)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 dadfe3338..43949c01a 100644 --- a/tests/shader_runner.c +++ b/tests/shader_runner.c @@ -1089,8 +1089,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 bda44a429..ee0e9094d 100644 --- a/tests/shader_runner.h +++ b/tests/shader_runner.h @@ -142,9 +142,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 a05dfd059..bb50564fa 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) @@ -561,6 +574,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, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473
From: Francisco Casas <fcasas(a)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 | 26 +++++++++++++++----------- tests/shader_runner.h | 6 ++++-- tests/shader_runner_d3d11.c | 8 ++++---- tests/shader_runner_d3d12.c | 14 +++++++------- tests/shader_runner_d3d9.c | 6 ++++-- tests/shader_runner_vulkan.c | 8 ++++---- 6 files changed, 38 insertions(+), 30 deletions(-) diff --git a/tests/shader_runner.c b/tests/shader_runner.c index 43949c01a..b9adec8b4 100644 --- a/tests/shader_runner.c +++ b/tests/shader_runner.c @@ -501,9 +501,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)) { @@ -940,7 +940,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; @@ -972,9 +972,9 @@ static void compile_shader(struct shader_runner *runner, IDxcCompiler3 *dxc_comp else { if (type == SHADER_TYPE_FX) - sprintf(profile, "%s_%s", shader_type_string(type), effect_models[runner->minimum_shader_model]); + sprintf(profile, "%s_%s", shader_type_string(type), effect_models[runner->selected_shader_model]); 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); @@ -1004,7 +1004,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; @@ -1021,17 +1021,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 @@ -1070,6 +1070,10 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o runner->ops = ops; runner->minimum_shader_model = minimum_shader_model; runner->maximum_shader_model = maximum_shader_model; + runner->selected_shader_model = minimum_shader_model; + + assert(runner->ops->check_requirements); + skip_tests = !runner->ops->check_requirements(runner, &runner->selected_shader_model); for (;;) { @@ -1089,9 +1093,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; } @@ -1249,6 +1252,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 ee0e9094d..89a01abff 100644 --- a/tests/shader_runner.h +++ b/tests/shader_runner.h @@ -122,6 +122,7 @@ struct shader_runner char *fx_source; enum shader_model minimum_shader_model; enum shader_model maximum_shader_model; + enum shader_model selected_shader_model; bool last_render_failed; @@ -142,8 +143,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 bb50564fa..fec1427b0 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; } @@ -334,7 +334,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; @@ -418,7 +418,7 @@ static bool d3d12_runner_draw(struct shader_runner *r, ps_code = compile_shader(runner, runner->r.ps_source, SHADER_TYPE_PS); vs_code = compile_shader(runner, runner->r.vs_source, SHADER_TYPE_VS); - todo_if(runner->r.is_todo && runner->r.minimum_shader_model < SHADER_MODEL_6_0) + todo_if(runner->r.is_todo && runner->r.selected_shader_model < SHADER_MODEL_6_0) ok(ps_code && vs_code, "Failed to compile shaders.\n"); if (!ps_code || !vs_code) 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); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473
From: Francisco Casas <fcasas(a)codeweavers.com> --- tests/shader_runner.c | 48 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/tests/shader_runner.c b/tests/shader_runner.c index b9adec8b4..745f17b3a 100644 --- a/tests/shader_runner.c +++ b/tests/shader_runner.c @@ -500,6 +500,10 @@ 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<4)", &line)) + runner->is_todo = runner->selected_shader_model < SHADER_MODEL_4_0; + else if (match_string(line, "todo(sm>=4)", &line)) + runner->is_todo = runner->selected_shader_model >= SHADER_MODEL_4_0; else if (match_string(line, "todo(sm<6)", &line)) runner->is_todo = runner->selected_shader_model < SHADER_MODEL_6_0; else if (match_string(line, "todo(sm>=6)", &line)) @@ -996,6 +1000,18 @@ static void compile_shader(struct shader_runner *runner, IDxcCompiler3 *dxc_comp } } +static enum parse_state get_parse_state_todo(enum parse_state state) +{ + if (state == STATE_SHADER_COMPUTE) + return STATE_SHADER_COMPUTE_TODO; + else if (state == STATE_SHADER_PIXEL) + return STATE_SHADER_PIXEL_TODO; + else if (state == STATE_SHADER_VERTEX) + return STATE_SHADER_VERTEX_TODO; + else + return STATE_SHADER_EFFECT_TODO; +} + static enum parse_state read_shader_directive(struct shader_runner *runner, enum parse_state state, const char *line, const char *src, HRESULT *expect_hr) { @@ -1006,19 +1022,35 @@ static enum parse_state read_shader_directive(struct shader_runner *runner, enum { if (runner->selected_shader_model >= SHADER_MODEL_6_0) continue; - if (state == STATE_SHADER_COMPUTE) - state = STATE_SHADER_COMPUTE_TODO; - else if (state == STATE_SHADER_PIXEL) - state = STATE_SHADER_PIXEL_TODO; - else if (state == STATE_SHADER_VERTEX) - state = STATE_SHADER_VERTEX_TODO; - else - state = STATE_SHADER_EFFECT_TODO; + state = get_parse_state_todo(state); + } + else if (match_directive_substring(src, "todo(sm<4)", &src)) + { + if (runner->selected_shader_model >= SHADER_MODEL_4_0) + continue; + state = get_parse_state_todo(state); + } + else if (match_directive_substring(src, "todo(sm>=4)", &src)) + { + if (runner->selected_shader_model >= SHADER_MODEL_6_0 + || runner->selected_shader_model < SHADER_MODEL_4_0) + continue; + state = get_parse_state_todo(state); } else if (match_directive_substring(src, "fail", &src)) { *expect_hr = E_FAIL; } + else if (match_directive_substring(src, "fail(sm<4)", &src)) + { + if (runner->selected_shader_model < SHADER_MODEL_4_0) + *expect_hr = E_FAIL; + } + else if (match_directive_substring(src, "fail(sm>=4)", &src)) + { + if (runner->selected_shader_model >= SHADER_MODEL_4_0) + *expect_hr = E_FAIL; + } else if (match_directive_substring(src, "fail(sm<6)", &src)) { if (runner->selected_shader_model < SHADER_MODEL_6_0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473
From: Francisco Casas <fcasas(a)codeweavers.com> Since we will keep doing one run through the test file for each call to run_shader_tests(), we will rely on intersecting the range of models that each runner is allowed to run with the range of models given in the test by the [require] directives. To simplify this, the minimum_test_model and maximum_test_model fields are renamed to minimum_test_model|maximum_test_model and will only keep track of the limits set by the [require] directive, and the won't be changed by the runners. For runners that we are interested in running on more than one mode range, specifically shader_runner_d3d12 and shader_runner_vulkan, we declare different fields to keep track of the range that will be focused on a given call. These fields were named minimum_runner_model and maximum_runner_model and were declared in their respective specializations of the shader_runner struct. They are used to properly implement the respective *_check_requirements() functions. Simpler runners like shader_runner_d3d9 and shader_runner_d3d11 that we will run only once, do not require these fields and all the logic can be implemented in the *_check_requirements() functions without additional fields. --- tests/shader_runner.c | 29 ++++++++++++----------------- tests/shader_runner.h | 7 +++---- tests/shader_runner_d3d11.c | 8 ++++---- tests/shader_runner_d3d12.c | 14 ++++++++++---- tests/shader_runner_d3d9.c | 6 +++--- tests/shader_runner_vulkan.c | 15 +++++++++++---- 6 files changed, 43 insertions(+), 36 deletions(-) diff --git a/tests/shader_runner.c b/tests/shader_runner.c index 745f17b3a..2494e2873 100644 --- a/tests/shader_runner.c +++ b/tests/shader_runner.c @@ -157,11 +157,11 @@ static void parse_require_directive(struct shader_runner *runner, const char *li { if (!i) fatal_error("Shader model < '%s' is invalid.\n", line); - runner->maximum_shader_model = min(runner->maximum_shader_model, i - 1); + runner->maximum_test_model = min(runner->maximum_test_model, i - 1); } else { - runner->minimum_shader_model = max(runner->minimum_shader_model, i); + runner->minimum_test_model = max(runner->minimum_test_model, i); } return; } @@ -1078,8 +1078,7 @@ static enum parse_state read_shader_directive(struct shader_runner *runner, enum return state; } -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) +void run_shader_tests(struct shader_runner *runner, const struct shader_runner_ops *ops, void *dxc_compiler) { size_t shader_source_size = 0, shader_source_len = 0; struct resource_params current_resource; @@ -1088,8 +1087,8 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o unsigned int i, line_number = 0; char *shader_source = NULL; HRESULT expect_hr = S_OK; - bool skip_tests = false; char line_buffer[256]; + bool skip_tests; FILE *f; if (!test_options.filename) @@ -1099,10 +1098,10 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o fatal_error("Unable to open '%s' for reading: %s\n", test_options.filename, strerror(errno)); memset(runner, 0, sizeof(*runner)); + runner->ops = ops; - runner->minimum_shader_model = minimum_shader_model; - runner->maximum_shader_model = maximum_shader_model; - runner->selected_shader_model = minimum_shader_model; + runner->minimum_test_model = SHADER_MODEL_2_0; + runner->maximum_test_model = SHADER_MODEL_6_0; assert(runner->ops->check_requirements); skip_tests = !runner->ops->check_requirements(runner, &runner->selected_shader_model); @@ -1125,11 +1124,8 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o break; case STATE_REQUIRE: - if (runner->maximum_shader_model < runner->minimum_shader_model - || !runner->ops->check_requirements(runner, &runner->selected_shader_model)) - { - skip_tests = true; - } + assert(runner->minimum_test_model <= runner->maximum_test_model); + skip_tests = !runner->ops->check_requirements(runner, &runner->selected_shader_model); break; case STATE_RESOURCE: @@ -1282,11 +1278,10 @@ void run_shader_tests(struct shader_runner *runner, const struct shader_runner_o else if (!strcmp(line, "[require]\n")) { 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->minimum_test_model = SHADER_MODEL_2_0; + runner->maximum_test_model = SHADER_MODEL_6_0; + skip_tests = !runner->ops->check_requirements(runner, &runner->selected_shader_model); runner->compile_options = 0; - skip_tests = false; } else if (match_directive_substring(line, "[pixel shader", &line)) { diff --git a/tests/shader_runner.h b/tests/shader_runner.h index 89a01abff..925385ac2 100644 --- a/tests/shader_runner.h +++ b/tests/shader_runner.h @@ -120,8 +120,8 @@ struct shader_runner char *ps_source; char *cs_source; char *fx_source; - enum shader_model minimum_shader_model; - enum shader_model maximum_shader_model; + enum shader_model minimum_test_model; + enum shader_model maximum_test_model; enum shader_model selected_shader_model; bool last_render_failed; @@ -167,8 +167,7 @@ void init_resource(struct resource *resource, const struct resource_params *para HRESULT dxc_compiler_compile_shader(void *dxc_compiler, enum shader_type type, unsigned int compile_options, 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); +void run_shader_tests(struct shader_runner *runner, const struct shader_runner_ops *ops, void *dxc_compiler); #ifdef _WIN32 void run_shader_tests_d3d9(void); diff --git a/tests/shader_runner_d3d11.c b/tests/shader_runner_d3d11.c index df704da8c..8c2075790 100644 --- a/tests/shader_runner_d3d11.c +++ b/tests/shader_runner_d3d11.c @@ -102,12 +102,12 @@ static bool d3d11_runner_check_requirements(struct shader_runner *r, enum shader { struct d3d11_shader_runner *runner = d3d11_shader_runner(r); - if (runner->r.maximum_shader_model < SHADER_MODEL_4_0) + if (runner->r.maximum_test_model < SHADER_MODEL_4_0) return false; - if (runner->r.minimum_shader_model >= SHADER_MODEL_6_0) + if (runner->r.minimum_test_model >= SHADER_MODEL_6_0) return false; - *model = max(SHADER_MODEL_4_0, runner->r.minimum_shader_model); + *model = max(SHADER_MODEL_4_0, runner->r.minimum_test_model); return true; } @@ -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); destroy_test_context(&runner); } } diff --git a/tests/shader_runner_d3d12.c b/tests/shader_runner_d3d12.c index fec1427b0..50af62c69 100644 --- a/tests/shader_runner_d3d12.c +++ b/tests/shader_runner_d3d12.c @@ -45,6 +45,9 @@ struct d3d12_shader_runner struct test_context test_context; + enum shader_model minimum_runner_model; + enum shader_model maximum_runner_model; + ID3D12DescriptorHeap *heap, *rtv_heap; ID3D12CommandQueue *compute_queue; @@ -98,13 +101,13 @@ static bool d3d12_runner_check_requirements(struct shader_runner *r, enum shader { struct d3d12_shader_runner *runner = d3d12_shader_runner(r); - if (runner->r.maximum_shader_model < SHADER_MODEL_4_0) + if (runner->r.maximum_test_model < runner->minimum_runner_model) return false; - if (runner->r.minimum_shader_model >= SHADER_MODEL_6_0 && !runner->dxc_compiler) + if (runner->r.minimum_test_model > runner->maximum_runner_model) return false; - *model = max(SHADER_MODEL_4_0, runner->r.minimum_shader_model); + *model = max(runner->r.minimum_test_model, runner->minimum_runner_model); return true; } @@ -616,7 +619,10 @@ void run_shader_tests_d3d12(void *dxc_compiler, enum shader_model minimum_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); + runner.minimum_runner_model = max(minimum_shader_model, SHADER_MODEL_4_0); + runner.maximum_runner_model = min(maximum_shader_model, SHADER_MODEL_6_0 - !dxc_compiler); + + run_shader_tests(&runner.r, &d3d12_runner_ops, dxc_compiler); 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 fbd7d6b5b..745c20228 100644 --- a/tests/shader_runner_d3d9.c +++ b/tests/shader_runner_d3d9.c @@ -187,10 +187,10 @@ static bool d3d9_runner_check_requirements(struct shader_runner *r, enum shader_ { struct d3d9_shader_runner *runner = d3d9_shader_runner(r); - if (runner->r.minimum_shader_model >= SHADER_MODEL_4_0) + if (runner->r.minimum_test_model >= SHADER_MODEL_4_0) return false; - *model = runner->r.minimum_shader_model; + *model = runner->r.minimum_test_model; return true; } @@ -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); destroy_test_context(&runner); } FreeLibrary(d3d9_module); diff --git a/tests/shader_runner_vulkan.c b/tests/shader_runner_vulkan.c index 131e394b0..483092686 100644 --- a/tests/shader_runner_vulkan.c +++ b/tests/shader_runner_vulkan.c @@ -56,6 +56,9 @@ struct vulkan_shader_runner { struct shader_runner r; + enum shader_model minimum_runner_model; + enum shader_model maximum_runner_model; + VkInstance instance; VkPhysicalDevice phys_device; VkDevice device; @@ -87,12 +90,13 @@ static bool vulkan_runner_check_requirements(struct shader_runner *r, enum shade { struct vulkan_shader_runner *runner = vulkan_shader_runner(r); - if (runner->r.maximum_shader_model < SHADER_MODEL_4_0) + if (runner->r.maximum_test_model < runner->minimum_runner_model) return false; - if (runner->r.minimum_shader_model >= SHADER_MODEL_6_0) + + if (runner->r.minimum_test_model > runner->maximum_runner_model) return false; - *model = max(SHADER_MODEL_4_0, runner->r.minimum_shader_model); + *model = max(runner->r.minimum_test_model, runner->minimum_runner_model); return true; } @@ -1397,7 +1401,10 @@ void run_shader_tests_vulkan(void) if (!init_vulkan_runner(&runner)) return; - run_shader_tests(&runner.r, &vulkan_runner_ops, NULL, SHADER_MODEL_2_0, SHADER_MODEL_5_1); + runner.minimum_runner_model = SHADER_MODEL_4_0; + runner.maximum_runner_model = SHADER_MODEL_5_1; + + run_shader_tests(&runner.r, &vulkan_runner_ops, NULL); cleanup_vulkan_runner(&runner); } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473
I still don't really understand what was wrong with 434? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_53370
I still don't really understand what was wrong with 434?
I don't feel strongly about specifying individual models vs. a range in the test file (although I don't see what's wrong with the former as long as we have a sensible default). We could just as easily construct the list based on a range, plus an optional "for this test we want to iterate over *all* shader models" flag (for something like semantics, where the interface changes in each sm1 version, or resource binding, where the interface changes between 5.0 and 5.1.) Most importantly, the fact that check_requirements is determining the selected shader model feels wrong to me, and fundamentally doesn't seem to fit well with the concept of running a file multiple times with different models. That logic shouldn't be in the backend file, it should be in the frontend if not in the shader file itself. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_53375
Most importantly, the fact that check_requirements is determining the selected shader model feels wrong to me, and fundamentally doesn't seem to fit well with the concept of running a file multiple times with different models. That logic shouldn't be in the backend file, it should be in the frontend if not in the shader file itself.
Agreed. The frontend requests to the backend to run a test with a given SM. The backend does it if it can, and doesn't do it if it cannot. WRT how the frontend is supposed to decide, I have some conflicting desires. Fundamentally I feel there are two concepts that make sense within a certain `.shader_test` file: * for which SMs this test even make sense, i.e., can be run and is expected to not fail (except for what `todo` directives allow); let's call this the _allowed_ SMs; * for which SMs we actually want to run this test in a default configuration; let's call this the _default_ SMs. A maximalist take would be to run on all the allowed SMs (which would make the default list meaningless), but in many cases that would be a waste and we probably don't want it. But at some level it still makes sense to have two different lists: the shader runner might have an option that overrides the default list, but the overriding list should still be compared with the allowed list. And of course having two lists might be too much complexity for the benefits they give. The current situation is that, in some sense, the `[require]` directive ranges implicitly define the allowed list, and the default list is computed by intersecting the allowed list with what the backend supports and taking the minimum; which I find an arbitrary and not particularly desirable algorithm. I see two ways to handle this: * we decide that we don't care about the allowed list: we only care about the default list and there is no need to have the option to manually select the SMs to run with, or at least we don't compare that against an allowed list; * we decide that we use ranges to specify the allowed list, and the default list is computed using some algorithm from the allowed list (e.g., intersection with `{2.0, 4.0, 6.0}`), but can be further tweaked with additional directives, for instance for adding a specific value to it or to rewrite it altogether; which is possibly similar to what Zeb is proposing. When we discussed that in St Paul I was leaning towards the first alternative, but I'm now finding the second to be more interesting. Or perhaps I'm just overanalyzing the problem. But, as stated above, I agree with Zeb that selecting the SM is not a backend's business. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_53501
I still don't really understand what was wrong with 434?
I don't feel strongly about specifying individual models vs. a range in the test file (although I don't see what's wrong with the former as long as we have a sensible default). We could just as easily construct the list based on a range,
Right, it's entirely orthogonal to adding support for generating/running d3dbc shaders to the Vulkan runner. We could certainly have a discussion about the merits of the different options, including perhaps supporting both at the same time, but I don't see the benefit of tying that discussion together with d3dbc support.
Most importantly, the fact that check_requirements is determining the selected shader model feels wrong to me, and fundamentally doesn't seem to fit well with the concept of running a file multiple times with different models. That logic shouldn't be in the backend file, it should be in the frontend if not in the shader file itself.
Agreed. The frontend requests to the backend to run a test with a given SM. The backend does it if it can, and doesn't do it if it cannot.
I disagree, and perhaps unsurprisingly in that regard, I don't think framing things in terms of "backend" and "frontend" is entirely fortunate. The way I look at it is that the different shader runners execute .shader_test files, in a number of different configurations, and we have some common parser (among others) infrastructure to help with that. It should be entirely up to individual runners to decide which configurations to run, and ideally this would be entirely opaque to the parser. And to make it a bit more concrete, some examples of different possible configuration properties: - Whether SPIR-V is used by the target API, or GLSL; that's particularly relevant to the OpenGL runner, of course. - The compiler used to compile HLSL: vkd3d-shader, d3dcompiler, or dxcompiler. - The target API: Vulkan, OpenGL, d3d9, d3d11, d3d12, etc. - When targetting GLSL, the version of the language we're targetting; there are significant differences between e.g. GLSL 1.20, 1.50, and 4.40. - Supported extensions/features; e.g. whether descriptor indexing is used, or whether we're using combined samplers. And there are of course many more of such properties. And while some of them are essentially tied together (e.g., we're not going to target GLSL on d3d12), I don't think it makes sense to burden the .shader_test parser with explicit knowledge about them, and which set of variants to run for a particular runner. Still, as interesting as that discussion may be, it's also largely irrelevant to the goal of generating/running d3dbc shaders. If the tests were to pass, it shouldn't be much harder than splitting the existing "run_shader_tests(&runner.r, &vulkan_runner_ops, NULL, SHADER_MODEL_2_0, SHADER_MODEL_5_1);" call into two separate ranges, and then making compile_shader() (both of them) actually compile 2.0 and 3.0 shaders as 2.0 and 3.0 shaders. But of course the tests aren't going to pass until we have support for compiling d3dbc to SPIR-V, and that's the only part of this that should be slightly complicated. We'll need to be able to support syntax like "todo(vk+sm<4) draw ...", and possibly syntax like "todo(vk+sm<4,sm>=6)", or some variant. We have a couple of different ways we could go about that: - We set some flag in struct shader_runner, e.g. "is_vk_d3dbc", extend the existing parsing for todo() and so on, and call it a day. The current memset() in run_shader_tests() doesn't help, but it shouldn't be too hard to either rework or work around. This may very well be the right option as far as d3dbc support is concerned. - We add something like "bool (*evaluate_condition)(struct shader_runner *runner, const char *condition);" to the shader_runner_ops structure, and use it to defer evaluating unrecognised condition strings to the runner. - We pass a configuration string to run_shader_tests(), and use that for things like todo and fail, as well as requirement checking. You could imagine the Vulkan runner passing e.g. "vk int64 rov", or perhaps even something like "vk sm>=4_0 sm<=5_1 int64 rov", and so on, getting rid of the existing "minimum_shader_model" and "maximum_shader_model" parameters. This is perhaps a little more complicated than the other two options, but not that much. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_53565
Most importantly, the fact that check_requirements is determining the selected shader model feels wrong to me, and fundamentally doesn't seem to fit well with the concept of running a file multiple times with different models. That logic shouldn't be in the backend file, it should be in the frontend if not in the shader file itself.
Agreed. The frontend requests to the backend to run a test with a given SM. The backend does it if it can, and doesn't do it if it cannot.
I disagree, and perhaps unsurprisingly in that regard, I don't think framing things in terms of "backend" and "frontend" is entirely fortunate. The way I look at it is that the different shader runners execute .shader_test files, in a number of different configurations, and we have some common parser (among others) infrastructure to help with that.
I don't understand this. My entire idea behind the shader runner was that it'd abstract away the difference between d3d APIs. But that just means that it abstracts away the implementation details. I don't see what's gained by giving individual runners the freedom to control *every* configuration decision, either conceptually or practically. Something like "are we using spirv?" certainly only makes sense in the context of one runner, but "shader model" is generic to the HLSL.
It should be entirely up to individual runners to decide which configurations to run, and ideally this would be entirely opaque to the parser.
How do we specify whether tests should be run with e.g. every shader model or just a few? I suspect your answer is "we don't; we run every test with every shader model". I haven't tried this but I worry this is going to become too many tests to run. Not least because I know Mesa has this problem. There's other things that fall into this same category of "don't inhere to the backend, matter for some tests but not all, and would probably result in too many configurations if we tested all of them all the time". Two examples are compilation flags, and d3dcompiler/d3dx9 version. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_53579
I disagree, and perhaps unsurprisingly in that regard, I don't think framing things in terms of "backend" and "frontend" is entirely fortunate. The way I look at it is that the different shader runners execute .shader_test files, in a number of different configurations, and we have some common parser (among others) infrastructure to help with that.
I don't understand this. My entire idea behind the shader runner was that it'd abstract away the difference between d3d APIs. But that just means that it abstracts away the implementation details. I don't see what's gained by giving individual runners the freedom to control *every* configuration decision, either conceptually or practically. Something like "are we using spirv?" certainly only makes sense in the context of one runner, but "shader model" is generic to the HLSL.
"Orthogonality is a good thing" is certainly a decent chunk of it, and conceptually I just don't think this is something that belongs in the parser, but ultimately I think it comes down to the fact that I think it works fine the way it currently works, and I don't see a compelling reason to change it. Perhaps that just means I'm oblivious to the problem, and perhaps that's something that was discussed in detail in person, but I did ask a couple of times, and didn't get a compelling answer. Incidentally, I'm not sure HLSL should be quite as central to the shader runner either; I could certainly imagine that being able to specify shaders as d3d-asm or one of the supported bytcode formats (including compiled effect files) would have its uses.
It should be entirely up to individual runners to decide which configurations to run, and ideally this would be entirely opaque to the parser.
How do we specify whether tests should be run with e.g. every shader model or just a few?
I suspect your answer is "we don't; we run every test with every shader model". I haven't tried this but I worry this is going to become too many tests to run. Not least because I know Mesa has this problem.
There's other things that fall into this same category of "don't inhere to the backend, matter for some tests but not all, and would probably result in too many configurations if we tested all of them all the time". Two examples are compilation flags, and d3dcompiler/d3dx9 version.
We invent some syntax for it and then implement it. E.g.: ``` [test] foreach (sm1,sm3,sm5) draw quad probe ... ... done ``` The specific syntax isn't terribly important at this point, but the parser would essentially just unroll the loop in that case. The broader, more important point though is that most of what's in this MR just shouldn't be needed for d3dbc support. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_53590
The specific syntax isn't terribly important at this point, but the parser would essentially just unroll the loop in that case.
It's not the syntax I'm trying to ask about, it's the implementation. The proposal here is that the parser gets a list of shader models (1, 3, 5), then runs the tests multiple times with each backend, passing one shader model at a time ("selected_shader_model"). If a shader model isn't supported by a backend [either inherently, like sm4 with d3d9, or because of missing features, like perhaps sm6 with Vulkan] then the test is skipped by check_requirements(). If we instead make shader model parsing the runner's responsibility, then as far as I can see the backend is going to have to do... pretty much the exact same thing, but in a more circumlocutious manner. I don't see the point of this, since I fail to anticipate any reason a runner would need more control over the way *HLSL* is directed to be compiled. And I fail to see why orthogonality is a concern since the decision is made pretty arbitrarily? Having things like the following makes sense to me: ``` run_tests_opengl_glsl(); run_tests_opengl_spirv(); run_tests_vulkan_spirv(); ``` and I can see the argument (if it's indeed being made) that anything else should be organized the same way. For things that matter to the runner (i.e. most of the items in your list) I think that's right, but I don't see that making sense for something that needs to be controlled by the input file. (And, if we say that there's a "default" list of selected_shader_models, probably (1, 4, 6), then we don't even need to invoke e.g. "d3d12 + sm4" and "d3d12 + sm6" separately at the top level anymore.)
The broader, more important point though is that most of what's in this MR just shouldn't be needed for d3dbc support.
No, it's not, and we could just take something like [1] which I wrote ages ago. But I think that more individual shader levels is something we're going to need sooner rather than later. I already have tests for sm1 semantics that would like it. [1], https://gitlab.winehq.org/zfigura/vkd3d/-/commit/f0a92809bb43eb0761985b4db3a... -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_53728
Please disregard my previous comment, which I deleted in gitlab but the e-mail was sent, as it was not finished yet. I pressed shift+enter in gitlab by mistake, which sends it immediately. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_54109
The specific syntax isn't terribly important at this point, but the parser would essentially just unroll the loop in that case.
It's not the syntax I'm trying to ask about, it's the implementation.
I'm not sure the implementation really is the interesting part, but see [the attached patch](/uploads/baab2c3a5fbb254e3de6d345579de56f/shader_runner_loops.diff) for a potential proof of concept. It takes some shortcuts, but I think it illustrates the basic concept that I had in mind well enough.
Having things like the following makes sense to me:
``` run_tests_opengl_glsl(); run_tests_opengl_spirv(); run_tests_vulkan_spirv(); ```
and I can see the argument (if it's indeed being made) that anything else should be organized the same way. For things that matter to the runner (i.e. most of the items in your list) I think that's right, but I don't see that making sense for something that needs to be controlled by the input file.
Actually, I'd even argue for just: ``` run_tests_opengl(); run_tests_vulkan(); ... ``` I.e., handling variants like GLSL/SPIR-V internally in the runners.
The broader, more important point though is that most of what's in this MR just shouldn't be needed for d3dbc support.
No, it's not, and we could just take something like \[1\] which I wrote ages ago. But I think that more individual shader levels is something we're going to need sooner rather than later. I already have tests for sm1 semantics that would like it.
\[1\], https://gitlab.winehq.org/zfigura/vkd3d/-/commit/f0a92809bb43eb0761985b4db3a...
I think the referenced commit is largely what I had in mind as far as d3dbc support is concerned, yes. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_54110
If I understand correctly, the biggest point of contention is that henri doesn't want the shader runners to lose control over the compilation:
It should be entirely up to individual runners to decide which configurations to run, and ideally this would be entirely opaque to the parser.
Because we may eventually want to support runners that don't use vkd3d-shader or the native HLSL compiler, or we may want to extend the shader_runner to other input formats. Currently we are compiling in the [shader] tests using the common shader_runner.c:compile_shader() and in the [test] tests using each runner's own compile_shader() function. And, all the proposals so far have in common that they extend the common compilation to SM1 and separate it from the runner's compilation in some way, and the problem is that giving more importance to this "common" compilation goes against those posibilities of extending the shader_runner. This is my proposal trying to find the common ground on the different approaches opinions: * Every parameter **we would pass** to the native compiler should be specified in the test and should not be decided by the runner, this includes the specific shader model and compilation flags that affect the result, such as matrix_majority. This makes sense because the expected hr and results are conditioned to these parameters. I am for specifying the shader models individually (and have defaults {2,4,6}) instead for working with model ranges, just for the sake of simplicity. But we can bikeshred the syntax to specify multiple shader models later. * But also, each runner should be in charge of the compilation (we need a shader_runner->compile function pointer field), it receives the shader model and the compilation parameters from the tests but it can also add additional parameters, or even decide to use another compiler altogether, the point is that it should aim to give the same hr as the native compiler would (otherwise it should be marked as "todo"), but most of them would rely on a default function that uses vkd3d-shader (or d3dcompiler_47.dll, if available) for SM<6 and dxcompiler for SM6 (if available). * We reroute the compilation tests in the [shader] directives to the runners, using the shader_runner->compile instead of calling the common shader_runner.c:compile_shader(), I don't see the need now of having these separated from the runners after this. * We also pass the shader model and compilation parameters to runner->check_requirements and we expect it to indicate whether it cannot run, it can run, or it can only compile. We use the latter to extend the Vulkan runner to SM1 but just compilation and not execution. Removing the burden of compilation to the parser and passing the parameters directly to the runners, is a step towards potentially extending the shader_runner for other input formats such as d3d-asm, as henri has mentioned. In this context, other formats may even be considered a special type of `enum shader_model`, in the sense that we could put it in the [require] directive. Naturally we may want to give `enum shader_model` another name. Having potentially more than one non-native way to compile means that we have to extend the `todo` syntax, this is also required for having more than one non-native way to run tests, so in this regard, I propose that: * We add a `runner->tag` string field to the runner's, which would be `vk` for vulkan, `gl` for opengl, etc. * We extend the todo() syntax so that both shader model ranges and runner tags can be specified, like `todo(vk,sm>=4,sm<6)`, we also allow multiple todo() markers so that several qualifiers in the same marker work as AND and several markers work as OR, e.g. `todo(vk,sm>=4,sm<6) todo(gl)` if we want to specify that the test doesn't work with gl yet and with vulkan only in SM4. * We add the following boolean fields to the runner `runner->ignore_compilation_todos`. When we are using a native compiler, we can set this flag true so that we ignore all the `todo` tags for [shader] tests. * Similarly, we add `runner->ignore_execution_todos` so that, if `true` all the `todo` tags are ignored in the [test] tests. This would be useful for the native runners (d3d9.dll, d3d11.dll, and d3d12.dll), alternatively, we could make the `todo()` syntax not include them by default, but I think this hardcoding can be avoided with this flag. * Similarly we extend the fail() syntax, but in this case it only needs shader model ranges. Among those changes, I can first implement the minimum necessary to get SM1 compilation with vkd3d-shader running (on the Vulkan runner), but knowing that we will go in that general direction. What do you think? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_54113
I'm not sure the implementation really is the interesting part, but see [the attached patch](https://gitlab.winehq.org/wine/vkd3d/uploads/baab2c3a5fbb254e3de6d345579de56...) for a potential proof of concept. It takes some shortcuts, but I think it illustrates the basic concept that I had in mind well enough.
Okay, I guess we must be talking past each other somehow, because that looks to me like it *is* putting that logic in the parser? It just decides the minimum == maximum for a run and feeds that through the runners. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_54136
Okay, I guess we must be talking past each other somehow,
Possibly, yes.
because that looks to me like it *is* putting that logic in the parser? It just decides the minimum == maximum for a run and feeds that through the runners.
Perhaps in a sense; I think conceptually it's somewhat different though. The idea here is to repeat/duplicate part of a test with different requirements/options; in principle there's no reason this needs to be limited to shader models and couldn't be extended to do something like this: ``` [pixel shader] ... [test] uniform 0 ... ... foreach (row-major column-major) draw quad (row-major) probe all rgba (0.5, 0.9, 0.6, 1.0) 1 (column-major) probe all rgba (0.2, 0.3, 0.6, 0.7) 1 done ``` The main practical difference is that the individual shader runners are responsible for invoking the parser with a particular configuration or set of capabilities, as opposed to the parser invoking the runners multiple times for entire .shader_test files. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_54239
Closing because it is superseded by !514. We shouldn't forget the important discussions here though! -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_55236
This merge request was closed by Francisco Casas. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473
The main practical difference is that the individual shader runners are responsible for invoking the parser with a particular configuration or set of capabilities, as opposed to the parser invoking the runners multiple times for entire .shader_test files.
I was rather confused at framing this as something the "individual runners" are doing since it's not like any runner logic is actually involved here, but IIUC I see now that you mean it should be done within a run rather than *as* an additional run. And yeah, I'm inclined to agree that makes more sense in general. (As I mentioned in IRC I still don't like the way we currently handle maximum_shader_model and minimum_shader_model, on the grounds of clarity. But yeah, it's not really necessary for sm1 tests.) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_55786
If I understand correctly, the biggest point of contention is that henri doesn't want the shader runners to lose control over the compilation:
It should be entirely up to individual runners to decide which configurations to run, and ideally this would be entirely opaque to the parser.
Because we may eventually want to support runners that don't use vkd3d-shader or the native HLSL compiler, or we may want to extend the shader_runner to other input formats.
I don't *think* this is what Henri was getting at. Nor do I think it's a particularly salient point: *if* we're going to test other input formats than HLSL we're going to necessarily need to invent new paths to do it. The interesting variables in question right now (shader model, d3dcompiler version) inhere to HLSL and don't inhere to any 3D API. For any other variables the opposite is true I think. FWIW, wrt asm, I think the right thing to do there is actually to compare the output directly, since there really should be no difference. I have some tests I've been working on along those lines.
* But also, each runner should be in charge of the compilation (we need a shader_runner->compile function pointer field), it receives the shader model and the compilation parameters from the tests but it can also add additional parameters, or even decide to use another compiler altogether, the point is that it should aim to give the same hr as the native compiler would (otherwise it should be marked as "todo"), but most of them would rely on a default function that uses vkd3d-shader (or d3dcompiler_47.dll, if available) for SM<6 and dxcompiler for SM6 (if available). * We reroute the compilation tests in the \[shader\] directives to the runners, using the shader_runner->compile instead of calling the common shader_runner.c:compile_shader(), I don't see the need now of having these separated from the runners after this.
Yeah, we could use some deduplication there probably.
* We also pass the shader model and compilation parameters to runner->check_requirements and we expect it to indicate whether it cannot run, it can run, or it can only compile. We use the latter to extend the Vulkan runner to SM1 but just compilation and not execution.
I don't really like that, it doesn't seem very declarative. I'd prefer to add a specific "compile-only" runner or pseudo-runner like this(?) patch series did, but we're also two short steps away from actual sm1 support so I don't think we need to bother anymore.
* We add a `runner->tag` string field to the runner's, which would be `vk` for vulkan, `gl` for opengl, etc. * We extend the todo() syntax so that both shader model ranges and runner tags can be specified, like `todo(vk,sm>=4,sm<6)`, we also allow multiple todo() markers so that several qualifiers in the same marker work as AND and several markers work as OR, e.g. `todo(vk,sm>=4,sm<6) todo(gl)` if we want to specify that the test doesn't work with gl yet and with vulkan only in SM4. * We add the following boolean fields to the runner `runner->ignore_compilation_todos`. When we are using a native compiler, we can set this flag true so that we ignore all the `todo` tags for \[shader\] tests. * Similarly, we add `runner->ignore_execution_todos` so that, if `true` all the `todo` tags are ignored in the \[test\] tests. This would be useful for the native runners (d3d9.dll, d3d11.dll, and d3d12.dll), alternatively, we could make the `todo()` syntax not include them by default, but I think this hardcoding can be avoided with this flag. * Similarly we extend the fail() syntax, but in this case it only needs shader model ranges.
Some of this can probably be hardcoded without needing to add unnecessary flexibility, but I'll wait until I read the patches. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_55787
The main practical difference is that the individual shader runners are responsible for invoking the parser with a particular configuration or set of capabilities, as opposed to the parser invoking the runners multiple times for entire .shader_test files.
I was rather confused at framing this as something the "individual runners" are doing since it's not like any runner logic is actually involved here, but IIUC I see now that you mean it should be done within a run rather than *as* an additional run. And yeah, I'm inclined to agree that makes more sense in general.
Right, the terminology perhaps isn't always clear. For clarity, this is roughly what I have been using: - "runners" for shader_runner_d3d12.c/shader_runner_vulkan.c/shader_runner_gl.c/etc. - "runner invocation" for run_shader_tests_d3d12()/run_shader_tests_vulkan()/run_shader_tests_gl()/etc. - "run" (or "parser invocation" above) for each run_shader_tests() call.
The interesting variables in question right now (shader model, d3dcompiler version) inhere to HLSL and don't inhere to any 3D API. For any other variables the opposite is true I think.
Although to some extent the supported shader model may also depend on e.g. the supported/selected d3d11 feature level.
FWIW, wrt asm, I think the right thing to do there is actually to compare the output directly, since there really should be no difference. I have some tests I've been working on along those lines.
If the goal is to test the assembler/disassembler, sure. I could imagine cases where authoring shaders in assembler or even bytecode may be more convenient than in HLSL though. A trivial case would perhaps be porting Wine d3d9 tests, where it would simply be easier to take the shaders as-is instead of converting them to HLSL, but we also have some tests like e.g. test_shader_instructions() that just want to test a particular sequence of instructions. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/473#note_55841
participants (5)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura (＠zfigura)