``` Subject: [PATCH 7/9] tests/shader-runner: Do not apply todo to result probes.
Test shaders do not touch upon any unimplemented features in vkd3d-shader and/or vkd3d, and vkd3d-shader should not emit incorrect SPIR-V. If a situation arises where probes are expected to fail, a 'todo' in a 'probe' line should set a different bool flag to be applied to the readback check only. ```
Removing the todo's is not wrong, but that commit message could have fooled me. What's actually happening is that since commit 26b89cc338c88e766d032f0ee3b6c9995dad02e8, probes are skipped when the corresponding draw fails; there's no point in probing after a failed draw, and if the render target contains uninitialised data it's just going to cause spurious failures. That means these todo's are now redundant. I don't think that necessarily justifies removing the ability to set a todo on probes though.
```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") ```
Our actual issue is with STDMETHODCALLTYPE in the generated header, right? We could potentially use #pragma push_macro/pop_macro for this, although perhaps ideally widl would have a way to deal with this.
```diff +/* Prevents a glitch in the ID3D10Blob_Release() macro when applied to an IDxcBlob. */ +#define WIDL_C_INLINE_WRAPPERS ```
Why is that? Is it because IDxcBlob has a different calling convention than ID3DBlob (we got rid of "ms_abi", after all), and the wrapper papers over the issue by causing the caller to put the blob pointer in the correct register?
```diff @@ -482,9 +499,17 @@ static void parse_test_directive(struct shader_runner *runner, const char *line) int ret;
runner->is_todo = false; + runner->is_todo_pipeline = false;
if (match_string(line, "todo", &line)) + { + runner->is_todo = runner->minimum_shader_model < SHADER_MODEL_6_0; + runner->is_todo_pipeline = runner->minimum_shader_model >= SHADER_MODEL_6_0; + } + else if (match_string(line, "todo(sm<6)", &line) && runner->minimum_shader_model < SHADER_MODEL_6_0) runner->is_todo = true; + else if (match_string(line, "todo(sm>=6)", &line) && runner->minimum_shader_model >= SHADER_MODEL_6_0) + runner->is_todo_pipeline = true;
if (match_string(line, "dispatch", &line)) { ```
I'm sorry, but it's still a mystery to me what this is trying to achieve. Why is the meaning of "todo" inverted for shader model 6.0+? Why do we need the separate "is_todo_pipeline" flag? Is this somehow related to not applying todo's to HLSL compilation when vkd3d-shader is not used for that? Couldn't we handle that with some minor adjustments to the corresponding "todo_if" conditions?
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.
Well, sure. You could even make an argument that "--dump-dxil" isn't all that critical to this MR either. It should largely just be a matter of calling D3DDisassemble() though. We don't have an implementation of that for vkd3d-utils yet, but we should, and we can largely just adopt the fairly straightforward implementation from Wine's d3dcompiler. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/346#note_46788