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.