Note that this MR is failing CI.
```diff +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 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.
```diff +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?
```diff +/* 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.
```diff +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.
```diff +typedef struct DxcDefine +{ + const WCHAR *Name; + const WCHAR *Value; +} DxcDefine; + ```
This seems to be unused.
```diff @@ -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: ```c 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.
```diff +#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(a)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? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/346#note_46664