tests/shader-runner: Test DXIL shaders.
This goes on top of MR 345.
Merge request reports
Activity
I haven't reviewed in depth yet, but for the moment I wouldn't mix shader runner tests between SM1-5 and SM6. The SM6 parser is still so crude that it fails on basically everything; also, running SM6 tests requires installing a separate binary component, which developers not interested about SM6 might not have. This would make the handling of
todo_6
and similar annotations more complicated, and for very little useful signal.For the moment I'd rather keep SM6 tests separated from all the others, and merge them once the SM6 parser allow a decent amount of them to pass.
Also, I would like to have some appropriate abstractions in the shader runner (and in the other vkd3d tests) so that SM6 support is not a special case everywhere, but rather the shader runner itself has support for having different shader compilers (vkd3d-shader, d3dcompiler_xx.dll, dxcompiler.dll) and for targeting different profiles in somehow the same way it now allows using different execution environments (that that support too is still to be perfected). Using SM6 and dxcompiler.dll should just be another case along these axes, rather than an exceptional code flow everywhere.
I had already written some code to this end, I can try to dust it off and submit a proposal. Of course perspectives on how that could work are welcome.
What do you think of all of this?
I haven't reviewed in depth yet, but for the moment I wouldn't mix shader runner tests between SM1-5 and SM6. The SM6 parser is still so crude that it fails on basically everything; also, running SM6 tests requires installing a separate binary component, which developers not interested about SM6 might not have. This would make the handling of
todo_6
and similar annotations more complicated, and for very little useful signal.For the moment I'd rather keep SM6 tests separated from all the others, and merge them once the SM6 parser allow a decent amount of them to pass.
I don't think that's a problem per se; I think it's fine to annotate some tests with todo/skip/fail based on e.g. the shader model or possible a specific runner. The CI can hopefully help avoid the need for each individual contributor to have a working dxcompiler installation, though I could certainly see it being a bit of a pain when adding new tests, certainly in the beginning when essentially nothing works.
I would like something a bit more coherent than "fail_tpf", "skip_dxil", and "todo_6" though. How about something along the lines of "fail(shader model < 6.0)" and "todo(shader model >= 6.0)"? "skip_dxil" sounds like it should simply be a "shader model < 6.0" line in the "[require]" section.
Also, I would like to have some appropriate abstractions in the shader runner (and in the other vkd3d tests) so that SM6 support is not a special case everywhere, but rather the shader runner itself has support for having different shader compilers (vkd3d-shader, d3dcompiler_xx.dll, dxcompiler.dll) and for targeting different profiles in somehow the same way it now allows using different execution environments (that that support too is still to be perfected). Using SM6 and dxcompiler.dll should just be another case along these axes, rather than an exceptional code flow everywhere.
Essentially, yes. I'm not necessarily convinced that we fully need to resolve that as part of this MR, but plumbing through a "use_dxcompiler" flag everywhere doesn't seem like a step in the right direction; I'd already feel a fair bit better about this if it was expressed in terms of minimum/maximum shader models. I.e., something along the lines of
run_shader_tests_d3d12(SHADER_MODEL_4_0, SHADER_MODEL_5_1); run_shader_tests_d3d12(SHADER_MODEL_6_0, SHADER_MODEL_6_0);
instead of
run_shader_tests_d3d12(false); run_shader_tests_d3d12(true);
There are a bunch of style issues in dxcompiler.idl, which I think should be fairly self-evident at this point. Beyond those though, why do we need that to be a public header?
The shader runner changes at first sight look like multiple changes in a single commit.
You'll probably want to use vkd3d_dlopen() and vkd3d_dlsym(), much like the Vulkan runner does.
It's perhaps worth pointing out that "8ba5fb08-5195-40e2-ac58-0d989c3a0102" is also the uuid for ID3D10Blob/ID3DBlob. It looks like IDxcBlob is a simple alias.
Beyond those though, why do we need that to be a public header?
I couldn't get it to use vkd3d_windows.h from within
tests
. I need to fix that at some point."skip_dxil" sounds like it should simply be a "shader model < 6.0" line in the "[require]" section.
Ironically I started with this and thought it may to too restrictive; I'll need to reorder the tests which need this so they come last in their source file, but it's rare enough not to be an issue.
Essentially, yes. I'm not necessarily convinced that we fully need to resolve that as part of this MR, but plumbing through a "use_dxcompiler" flag everywhere doesn't seem like a step in the right direction; I'd already feel a fair bit better about this if it was expressed in terms of minimum/maximum shader models. I.e., something along the lines of
run_shader_tests_d3d12(SHADER_MODEL_4_0, SHADER_MODEL_5_1); run_shader_tests_d3d12(SHADER_MODEL_6_0, SHADER_MODEL_6_0);
instead of
run_shader_tests_d3d12(false); run_shader_tests_d3d12(true);
Yeah, that's already better. It still conflates the SM and the compiler, which is a thing I'd like to eventually disentangle, but that doesn't need to happen in this MR, and I can take responsibility for that.
I would like something a bit more coherent than "fail_tpf", "skip_dxil", and "todo_6" though. How about something along the lines of "fail(shader model < 6.0)" and "todo(shader model >= 6.0)"? "skip_dxil" sounds like it should simply be a "shader model < 6.0" line in the "[require]" section.
I agree something more coherent would be nice, but that seems excessively verbose to me. I don't think we're going to need any more complex categories than "sm1", "sm4", and "sm6".
Wrt fail_tpf... what may make sense is, every time we have a require directive for a shader version, we try to compile it with every version anyway, and validate that HLSL compilation fails for versions outside of the range. Then that would become
[require] shader model >= 6.0
Although you do mention that some tests ICE the native compiler, so I'm not sure how to deal with that...
Somewhat relatedly, for some of those tests I don't even know how the relevant shader would be interpreted, so we may want an actual [test] directive following them.
"skip_dxil" sounds like it should simply be a "shader model < 6.0" line in the "[require]" section.
Ironically I started with this and thought it may to too restrictive; I'll need to reorder the tests which need this so they come last in their source file, but it's rare enough not to be an issue.
No, we should probably just change [require] so that it doesn't skip the whole file.
That or move the relevant tests to separate files. Embedding lots of tests in the same file is... the problem with it is that, somewhat unlike C, it's not the easiest thing to read; I blame the lack of syntax highlighting support.
I agree something more coherent would be nice, but that seems excessively verbose to me. I don't think we're going to need any more complex categories than "sm1", "sm4", and "sm6".
I favour less verbosity too. It can be, e.g.
todo(sm4)
or a list liketodo(sm4,sm6)
, with plaintodo
applying to all.
added 4 commits
Toggle commit listadded 12 commits
- 1805aa58...80ff1f87 - 2 earlier commits
- db54961c - vkd3d-shader/dxbc: Load input signatures also from ISG1 chunks.
- 0613680a - vkd3d-shader/dxil: Declare shader inputs.
- 2e071572 - vkd3d-shader/dxil: Implement DX instruction LoadInput.
- 0091f197 - vkd3d-shader/dxil: Do not access null code blocks on failure.
- f79b0096 - vkd3d-shader/dxil: Do not compile compute shaders.
- 2b685f78 - tests/shader-runner: Handle individual keywords in shader directives.
- e2360dcc - tests/shader-runner: Do not exit if a 'require' directive is not met.
- 91ca0c28 - tests/shader-runner: Replace immediate shader type strings with an enum.
- bf5eb7d7 - tests/shader-runner: Test shaders with dxcompiler.
- cc5ac663 - tests/shader-runner: Add a '--dump-dxil' command line switch.
Toggle commit listadded 5 commits
- 90652afc - tests/shader-runner: Handle individual keywords in shader directives.
- 57126795 - tests/shader-runner: Do not exit if a 'require' directive is not met.
- f5d668c2 - tests/shader-runner: Replace immediate shader type strings with an enum.
- 5fcd94b3 - tests/shader-runner: Test shaders with dxcompiler.
- 193394ef - tests/shader-runner: Add a '--dump-dxil' command line switch.
Toggle commit listadded 27 commits
-
738d134b...46c7f65b - 15 commits from branch
wine:master
- 46c7f65b...baaf7b6e - 2 earlier commits
- 5d9fa52b - vkd3d-shader/spirv: Introduce a Static Single Assignment register type.
- 4df09e6e - vkd3d-shader/spirv: Build undefined values once.
- 7b91e979 - vkd3d-shader/dxbc: Load input signatures also from ISG1 chunks.
- 266ba19f - vkd3d-shader/dxil: Declare shader inputs.
- 6badb521 - vkd3d-shader/dxil: Implement DX instruction LoadInput.
- 6a58ef7b - tests/shader-runner: Handle individual keywords in shader directives.
- b7376f6e - tests/shader-runner: Do not exit if a 'require' directive is not met.
- 1ab4ebd5 - tests/shader-runner: Replace immediate shader type strings with an enum.
- 64ab31f2 - tests/shader-runner: Test shaders with dxcompiler.
- d78ac338 - tests/shader-runner: Add a '--dump-dxil' command line switch.
Toggle commit list-
738d134b...46c7f65b - 15 commits from branch
added 12 commits
- d78ac338...7975227d - 2 earlier commits
- 873c9a4e - vkd3d-shader/spirv: Introduce a Static Single Assignment register type.
- ab37b6e2 - vkd3d-shader/spirv: Build undefined values once.
- 9e073ce5 - vkd3d-shader/dxbc: Load input signatures also from ISG1 chunks.
- 76e59dc5 - vkd3d-shader/dxil: Declare shader inputs.
- f2e6f2e4 - vkd3d-shader/dxil: Implement DX instruction LoadInput.
- aff611e1 - tests/shader-runner: Handle individual keywords in shader directives.
- 89a8164d - tests/shader-runner: Do not exit if a 'require' directive is not met.
- 64e3baeb - tests/shader-runner: Replace immediate shader type strings with an enum.
- fbb789cd - tests/shader-runner: Test shaders with dxcompiler.
- 9b36e00a - tests/shader-runner: Add a '--dump-dxil' command line switch.
Toggle commit listadded 46 commits
-
3b7239d5...ca05e57e - 34 commits from branch
wine:master
- ca05e57e...262a1671 - 2 earlier commits
- 0943bd0f - vkd3d-shader/spirv: Introduce a Static Single Assignment register type.
- 2fbe46d8 - vkd3d-shader/spirv: Build undefined values once.
- 6c5a9747 - vkd3d-shader/dxbc: Load input signatures also from ISG1 chunks.
- 8832d895 - vkd3d-shader/dxil: Declare shader inputs.
- 16ebb250 - vkd3d-shader/dxil: Implement DX instruction LoadInput.
- 26d00971 - tests/shader-runner: Handle individual keywords in shader directives.
- 1de4c03b - tests/shader-runner: Do not exit if a 'require' directive is not met.
- 01f87a17 - tests/shader-runner: Replace immediate shader type strings with an enum.
- 28310221 - tests/shader-runner: Test shaders with dxcompiler.
- 0afea9cd - tests/shader-runner: Add a '--dump-dxil' command line switch.
Toggle commit list-
3b7239d5...ca05e57e - 34 commits from branch
mentioned in merge request !345 (merged)
Note that this MR is failing CI.
+enum shader_type +{ + SHADER_TYPE_CS, + SHADER_TYPE_GS, + SHADER_TYPE_HS, + SHADER_TYPE_PS, + SHADER_TYPE_VS, +};
This effectively adds hull and geometry shaders. I suppose that's fine, although it does mean the commit does a bit more than the commit message suggests. But if we're going to add those, why not add domain shaders as well then?
diff --git a/tests/dxcompiler.idl b/tests/dxcompiler.idl new file mode 100644 index 000000000..83b0ed5cd --- /dev/null +++ b/tests/dxcompiler.idl @@ -0,0 +1,155 @@ +import "vkd3d_windows.h"; +#include "vkd3d_unknown.idl"
That's missing our usual copyright header.
+cpp_quote("#ifdef _WIN32") +cpp_quote("#ifndef DXC_API_IMPORT") +cpp_quote("#define DXC_API_IMPORT __declspec(dllimport)") +cpp_quote("#endif") +cpp_quote("#else") +cpp_quote("#ifndef DXC_API_IMPORT") +cpp_quote("#define DXC_API_IMPORT __attribute__ ((visibility (\"default\")))") +cpp_quote("#endif") +cpp_quote("#endif")
I don't think these are used?
+/* Strip __attribute__((ms_abi)) defined in vkd3d_windows.h as dxcompiler does not use it. */ +cpp_quote("#ifdef __x86_64__") +cpp_quote("# undef __stdcall") +cpp_quote("# define __stdcall") +cpp_quote("#endif")
That's going to affect subsequent code as well.
+interface IDxcBlobEncoding : IDxcBlob +{ + HRESULT GetEncoding(BOOL *pKnown, UINT32 *pCodePage); +}
That's not our usual convention for parameter names. Affects IDxcIncludeHandler, IDxcOperationResult, IDxcResult, and IDxcCompiler3 as well.
+typedef struct DxcDefine +{ + const WCHAR *Name; + const WCHAR *Value; +} DxcDefine; +
This seems to be unused.
@@ -478,13 +493,22 @@ static void read_uint4(const char **line, struct uvec4 *v) static void parse_test_directive(struct shader_runner *runner, const char *line) { + bool use_dxcompiler = runner->minimum_shader_model >= SHADER_MODEL_6_0; char *rest; int ret; runner->is_todo = false; + runner->is_todo_6 = false; if (match_string(line, "todo", &line)) + { + runner->is_todo = !use_dxcompiler; + runner->is_todo_6 = use_dxcompiler; + } + else if (match_string(line, "todo(sm<6)", &line) && !use_dxcompiler) runner->is_todo = true; + else if (match_string(line, "todo(sm>=6)", &line) && use_dxcompiler) + runner->is_todo_6 = true; if (match_string(line, "dispatch", &line)) {
That just looks confusing to me. What I'd expect here is essentially something like this:
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; else if (match_string(line, "todo(sm>=6)", &line)) runner->is_todo = runner->minimum_shader_model >= SHADER_MODEL_6_0;
Something similar comes up in a number of other places as well. Fundamentally, I don't think the shader runner should be concerned about where a particular backend runner is using dxcompiler or not.
+#ifdef _WIN32 +static const char dxcompiler_name[] = "dxcompiler.dll"; +#else +static const char dxcompiler_name[] = "libdxcompiler.so"; +#endif
We could probably introduce SONAME_LIBDXCOMPILER for this.
From 5635ad98aa3303a1a7d00080d63afe8f4d683747 Mon Sep 17 00:00:00 2001 From: Conor McCarthy <cmccarthy@codeweavers.com> Date: Thu, 14 Sep 2023 19:42:57 +1000 Subject: [PATCH 12/12] tests/shader-runner: Add a '--dump-dxil' command line switch. Dumps DXIL disassembly when shaders are compiled with dxcompiler.
I suppose that's fine. I wonder though, does this need to be specific to DXIL? If this is useful, I don't think there's a reason we couldn't support dumping disassembly for shaders compiled by d3dcompiler or vkd3d-shader as well?
I suppose that's fine. I wonder though, does this need to be specific to DXIL? If this is useful, I don't think there's a reason we couldn't support dumping disassembly for shaders compiled by d3dcompiler or vkd3d-shader as well?
It's especially useful for DXIL because it's under development. Maybe that could apply to d3dbc too, but I don't think plumbing a shader dump for other binaries would be simple enough to be part of this MR.
added 28 commits
-
5635ad98...d9c8b49e - 19 commits from branch
wine:master
- fe0c6a25 - vkd3d-shader/dxil: Do not access null code blocks on failure.
- 392892f3 - vkd3d-shader/dxil: Do not compile compute shaders.
- 50107897 - vkd3d-shader/dxil: Convert into an error the warning for an unhandled instrinsic.
- b78381d8 - tests/shader-runner: Handle individual keywords in shader directives.
- 4888e2c7 - tests/shader-runner: Do not exit if a 'require' directive is not met.
- 70f83a11 - tests/shader-runner: Replace immediate shader type strings with an enum.
- fe2eb749 - tests/shader-runner: Do not apply todo to result probes.
- 0cf4b5d6 - tests/shader-runner: Test shaders with dxcompiler.
- 4f25a5ce - tests/shader-runner: Add a '--dump-dxil' command line switch.
Toggle commit list-
5635ad98...d9c8b49e - 19 commits from branch
I renamed
is_todo_6
tois_todo_pipeline
to reflect that it has a different meaning thanis_todo
.I added a new patch to remove the application of
is_todo
to result probes. The reasoning in explained in the commit.dxcompiler sometimes emits a binary on failure. In some cases it reflects the erroneous HLSL, e.g. when
numthreads[(0, 1, 1)]
is used, but in others the binary does something else, e.g. when uniforms are used in themain()
parameter list it loads them as signature inputs. TheIDxcResult
parentIDxcOperationResult
interface contains aGetStatus()
function which returns correct failure information.Many shaders use uniform parameters, so these now have
fail(sm>=6)
tags. A later patch should probably move all of these uniforms out of the parameter list, and add a single test file to test uniform parameters in SM < 6. We don't need this yet as uniforms require loading of resource info and implementation of cbuffer loads.This MR now depends on MR 345, mainly to make it unnecessary to have todos on result probes where DX intrinsics are unimplemented (i.e. where
emit_unhandled()
is called).A test in
entry-point-semantics.shader_test
fails because it depends on the DXIL shader signature. We could skip it or wait for the next MR which will load signatures.