On Tue Oct 25 11:40:38 2022 +0000, Henri Verbeet wrote:
My feeling is that we shouldn't accept a SM6 shader if the user
specified a SM4 input. We should provide a new `VKD3D_SHADER_SOURCE_DXBC_DXIL` value in `enum vkd3d_shader_source_type` and accept a SM6 shader if that value was specified. Yes, agreed.
Out of convenience, we could also add something like
`VKD3D_SHADER_SOURCE_DXBC`, which would allow any DXBC shader to be accepted. And possibly generalize it even further in order to autodetect SM1 too. I don't know if this would be useful for something. Not at this level. We do have autodetection code in vkd3d-compiler, and an argument could perhaps be made for making vkd3d-shader API available for that, but we probably don't want to do that in vkd3d_shader_compile()/vkd3d_shader_scan(). I've only had a somewhat cursory look at this series, but I did notice a couple of higher level things:
- This uses vkd3d_shader_parser, but doesn't actually use the existing
vkd3d_shader_parser interfaces. Most notably, this doesn't implement parser_read_instruction, and instead introduces parser_read_module. Changing the vkd3d_shader_parser interface to accommodate DXIL is fine of course, but then the other users/implementations of that interface need to be adjusted as well. Similarly, we don't want types specific to a particular implementation like vkd3d_shader_sm6_module in that interface.
- Somewhat similarly, this introduces variants of existing structures
and enumerations like vkd3d_shader_instruction and vkd3d_shader_opcode. Perhaps that makes sense, but it's not obvious from this series what the motivation behind that is, or that this couldn't be reconciled with the existing infrastructure.
- The individual patches in this series introduce quite a lot of code
at once, and it doesn't seem to hard to split these patches.
- There's a conspicuous lack of vkd3d_shader_error() or similar calls
in these patches. I.e., this generally uses ERR/FIXME/WARN for messages that should instead get propagated to the application.
In addition to instructions, much extra information needs to be passed out to the tracer or compiler, and parser_read_instruction is not practical for this. Instructions also need to reference other instructions, and the practice of writing one instruction to a struct would make that messy. Should vkd3d_shader_parser not be used at all for SM 6?
I initially was translating instructions to use vkd3d_shader_opcode, but since none can be handled by common code, we would end up using the same enum for two different things, with numerous values being used in one implementation but not the other. Also there would be inefficiencies, e.g. all numeric instructions with two inputs are emitted using a single BINOP with the actual operation encoded as an operand. We would translate this to one of the numerous VKD3DSIH equivalents, only to handle them all later in a single case statement.
It's possible to add another union member to vkd3d_shader_instruction for SM 6, but of the other fields only handler_idx, flags and src_count would be used. Use of common code would be rare; vkd3d_dxbc_compiler_map_ext_glsl_instruction() and vkd3d_dxbc_compiler_map_atomic_instruction() are the only two I found in a brief search, and the translation overhead probably cancels any gain.
I could reduce the large patch to nothing more than reading the block tree. This wouldn't do anything except validate the blocks and emit any errors encountered. Is that enough, or too much?