vkd3d-shader: Store shader IR instructions in an array.
Storing in an array facilitates normalisation passes on the IR, and also avoids needing to store complex state information in a future DXIL parser.
Merge request reports
Activity
added 1 commit
- cc9a3702 - vkd3d-shader: Merge all shader fork and join phases into a single phase.
added 1 commit
- c5032973 - vkd3d-shader: Merge all shader fork and join phases into a single phase.
Broadly this seems in the right direction. Patch 1/2 should be split though; for example:
- Parse d3dbc bytecode up-front into a shader instruction array. This could keep the existing vkd3d_shader_parser interface, and just iterate over the instruction in the array in shader_sm1_read_instruction(). shader_sm1_reset() and shader_sm1_is_end() would have obvious implementations as well.
- Do the same for vkd3d_shader_sm4_parser.
- Use the instruction array interface in vkd3d_dxbc_binary_to_text()
- Likewise for scan_with_parser().
- The GLSL backend.
- The SPIR-V backend.
That's 6 separate parts right there; it seems plausible that the vkd3d_shader_sm1_parser and vkd3d_shader_sm4_parser parts could be further split, but I haven't looked that close yet.
+ if (!shader_instruction_array_reserve(instructions, 1)) + { + ... + } + ins = &instructions->elements[instructions->count];
shader_instruction_array_reserve() seems a bit awkward, in part because of the similarity in name to vkd3d_array_reserve(), which gets passed the total size to reserve. It might be nicer to do something like "shader_instruction_array_reserve(instructions, instructions->count + 1)".
+struct vkd3d_shader_instruction_array +{ + struct vkd3d_shader_instruction *elements; + size_t capacity; + unsigned int count; + + struct vkd3d_shader_param_allocator src_params; + struct vkd3d_shader_param_allocator dst_params; +};
It seems odd to use different types for "capacity" and "count" above. Could we use size_t for both?
Patch 2/2 is a "vkd3d-shader/spirv" patch. I'd probably reframe it as doing the existing transformation on the vkd3d_shader_instruction IR instead of during translation to SPIR-V.
- while (!vkd3d_shader_parser_is_end(parser)) - { - vkd3d_shader_parser_read_instruction(parser, &instruction); - - if (instruction.handler_idx == VKD3DSIH_INVALID) - { - WARN("Encountered unrecognized or invalid instruction.\n"); - ret = VKD3D_ERROR_INVALID_SHADER; - break; - } - - if ((ret = spirv_compiler_handle_instruction(spirv_compiler, &instruction)) < 0) - break; - } - - if (parser->failed) - ret = VKD3D_ERROR_INVALID_SHADER; + ret = spirv_compiler_handle_instructions(spirv_compiler, &parser->instructions); if (ret >= 0) ret = spirv_compiler_generate_spirv(spirv_compiler, compile_info, out);
It's somewhat tangential to this series, but I think we should pass "parser" to spirv_compiler_generate_spirv(), and then just fold spirv_compiler_handle_instructions() into spirv_compiler_generate_spirv(), analogous to vkd3d_glsl_generator_generate().
added 9 commits
- 7bc85e0d - vkd3d-shader/sm1: Store parsed instructions in an array.
- 473a5bea - vkd3d-shader/sm4: Store parsed instructions in an array.
- 84248e18 - vkd3d-shader: Initialise the instruction array in vkd3d_shader_parser_init().
- 7cb78117 - vkd3d-shader/trace: Use the instruction array interface in vkd3d_dxbc_binary_to_text().
- 8d117927 - vkd3d-shader: Use the instruction array interface in scan_with_parser().
- ce6279d0 - vkd3d-shader/glsl: Use the instruction array interface in vkd3d_glsl_generator_generate().
- 25f1ebe7 - vkd3d-shader/sm4: Use the instruction array interface in compile_dxbc_tpf().
- 5858b0ed - vkd3d-shader/spirv: Pass a parser pointer to spirv_compiler_generate_spirv().
- 9ce5b1a7 - vkd3d-shader/spirv: Merge all shader IR fork and join phases into a single phase.
Toggle commit listadded 1 commit
- fd00a93c - vkd3d-shader/spirv: Merge all shader IR fork and join phases into a single phase.
I did a fairly detailed review of the first two patches in this series, and found a couple of mostly minor issues. I also did a more cursory review of the subsequent 6 patches, and at first sight those seem fine. With the issues fixed, the first 8 patches in this series are probably ready to go in, although I'd like either @giomasce or @zfigura to approve those as well.
I haven't looked at the last patch in the series in detail yet; it would probably make sense to submit the first 8 patches as a separate MR after fixing the issues.
+static struct vkd3d_shader_param_node *shader_param_allocator_node_create( + struct vkd3d_shader_param_allocator *allocator) +{ + struct vkd3d_shader_param_node *node; + + if (!(node = vkd3d_malloc(sizeof(*node) + allocator->count * allocator->stride))) + return NULL; + node->next = NULL; + return node; +}
It doesn't bother me too much, but we'd normally write that allocation as:
vkd3d_malloc(offsetof(struct vkd3d_shader_param_node, param[allocator->count * allocator->stride]))
+static bool shader_param_allocator_init(struct vkd3d_shader_param_allocator *allocator, + unsigned int count, unsigned int stride) +{ + allocator->count = max(count, 4); + allocator->stride = stride; + allocator->head = shader_param_allocator_node_create(allocator); + allocator->current = allocator->head; + allocator->index = 0; + + return true; +}
The return value of shader_param_allocator_node_create() is unchecked above.
I think there's an argument for just passing the size to shader_param_allocator_node_create(). I.e.:
count = max(count, 4); allocator->count = count; ... allocator->head = shader_param_allocator_node_create(count * stride);
mostly because that would avoid passing around a pointer to "allocator" while it's still being constructed. That does slightly complicate shader_param_allocator_get() though, so perhaps the choice is not completely straightforward.
+void *shader_param_allocator_get(struct vkd3d_shader_param_allocator *allocator, unsigned int count) +{ + void *params; + + if (!count) + return NULL; + + if (allocator->index + count > allocator->count) + { + struct vkd3d_shader_param_node *next = shader_param_allocator_node_create(allocator); + allocator->current->next = next; + allocator->current = next; + allocator->index = 0; + } + + params = &allocator->current->param[allocator->index * allocator->stride]; + allocator->index += count; + return params; +}
The expression "allocator->index + count" can overflow. Rewriting the condition as "if (count > allocator->count - allocator->index)" avoids that issue.
The return value of shader_param_allocator_node_create() is unchecked.
+bool shader_instruction_array_init(struct vkd3d_shader_instruction_array *instructions, unsigned int reserve) +{ + memset(instructions, 0, sizeof(*instructions)); + /* Size the parameter initial allocations so they are large enough for most shaders. */ + return shader_instruction_array_reserve(instructions, reserve) + && shader_param_allocator_init(&instructions->dst_params, reserve - reserve / 8u, sizeof(*instructions->elements->dst)) + && shader_param_allocator_init(&instructions->src_params, reserve * 2u, sizeof(*instructions->elements->src)); +}
This would leak "instructions->elements" and "instructions->dst_params.head" if the second shader_param_allocator_init() call were to fail.
+void shader_instruction_array_destroy(struct vkd3d_shader_instruction_array *instructions, bool destroy_instructions) +{ + unsigned int i; + + if (destroy_instructions) + { + for (i = 0; i < instructions->count; ++i) + if (instructions->elements[i].handler_idx == VKD3DSIH_DCL_IMMEDIATE_CONSTANT_BUFFER) + vkd3d_free((void *)instructions->elements[i].declaration.icb); + } + vkd3d_free(instructions->elements); + shader_param_allocator_destroy(&instructions->dst_params); + shader_param_allocator_destroy(&instructions->src_params); +}
The "destroy_instructions" parameter is not used until much later in the series. I also think it's a little ugly; it may be preferable to just copy "icb" in shader_instruction_array_clone_instruction(). Alternatively, we could store a flag in the vkd3d_shader_instruction structure to indicate whether we have a "cloned" instruction or not.
Note that with the current scheme there's also a potential issue with reusing the original "icb" storage for VKD3DSIH_DCL_IMMEDIATE_CONSTANT_BUFFER instructions: creating new VKD3DSIH_DCL_IMMEDIATE_CONSTANT_BUFFER instructions in the normaliser would require new allocations for "icb", which would then be leaked by shader_instruction_array_destroy().
@@ -206,7 +200,8 @@ static bool shader_sm4_read_register_space(struct vkd3d_shader_sm4_parser *priv, static void shader_sm4_read_conditional_op(struct vkd3d_shader_instruction *ins, uint32_t opcode, uint32_t opcode_token, const uint32_t *tokens, unsigned int token_count, struct vkd3d_shader_sm4_parser *priv) { - shader_sm4_read_src_param(priv, &tokens, &tokens[token_count], VKD3D_DATA_UINT, &priv->src_param[0]); + shader_sm4_read_src_param(priv, &tokens, &tokens[token_count], VKD3D_DATA_UINT, + (struct vkd3d_shader_src_param *)&ins->src[0]); ins->flags = (opcode_token & VKD3D_SM4_CONDITIONAL_NZ) ? VKD3D_SHADER_CONDITIONAL_OP_NZ : VKD3D_SHADER_CONDITIONAL_OP_Z; }
Do we need that cast?
@@ -227,16 +223,23 @@ static void shader_sm4_read_shader_data(struct vkd3d_shader_instruction *ins, ui ... + if (!(icb = vkd3d_malloc(sizeof(*icb) + sizeof(*tokens) * icb_size)))
Like further above, we'd typically use
offsetof
to calculate the allocation size here.@@ -438,8 +441,9 @@ static void shader_sm4_read_dcl_global_flags(struct vkd3d_shader_instruction *in static void shader_sm5_read_fcall(struct vkd3d_shader_instruction *ins, uint32_t opcode, uint32_t opcode_token, const uint32_t *tokens, unsigned int token_count, struct vkd3d_shader_sm4_parser *priv) { - priv->src_param[0].reg.u.fp_body_idx = *tokens++; - shader_sm4_read_src_param(priv, &tokens, &tokens[token_count], VKD3D_DATA_OPAQUE, &priv->src_param[0]); + struct vkd3d_shader_src_param *src_params = (struct vkd3d_shader_src_param *)ins->src; + src_params[0].reg.u.fp_body_idx = *tokens++; + shader_sm4_read_src_param(priv, &tokens, &tokens[token_count], VKD3D_DATA_OPAQUE, &src_params[0]); } static void shader_sm5_read_dcl_function_body(struct vkd3d_shader_instruction *ins, uint32_t opcode,
Like above, do we need this cast?
1648 allocator->current->next = next; 1649 allocator->current = next; 1650 allocator->index = 0; 1651 } 1652 1653 params = &allocator->current->param[allocator->index * allocator->stride]; 1654 allocator->index += count; 1655 return params; 1656 } 1657 1658 bool shader_instruction_array_init(struct vkd3d_shader_instruction_array *instructions, unsigned int reserve) 1659 { 1660 memset(instructions, 0, sizeof(*instructions)); 1661 /* Size the parameter initial allocations so they are large enough for most shaders. */ 1662 return shader_instruction_array_reserve(instructions, reserve) 1663 && shader_param_allocator_init(&instructions->dst_params, reserve - reserve / 8u, sizeof(*instructions->elements->dst)) That's a completely minor thing, but maybe you can use just
reserve
here. The different withreserve - reserve / 8u
shouldn't be that important, and, even with the comment, using such a specific expression leaves the reader asking what they're missing IMHO.Edited by Giovanni Mascellanichanged this line in version 6 of the diff
added 8 commits
- 9f9f2282 - vkd3d-shader/sm1: Store parsed instructions in an array.
- 93616a37 - vkd3d-shader/sm4: Store parsed instructions in an array.
- bab03825 - vkd3d-shader: Initialise the instruction array in vkd3d_shader_parser_init().
- 92cfeec1 - vkd3d-shader/trace: Use the instruction array interface in vkd3d_dxbc_binary_to_text().
- a7649670 - vkd3d-shader: Use the instruction array interface in scan_with_parser().
- 971fe615 - vkd3d-shader/glsl: Use the instruction array interface in vkd3d_glsl_generator_generate().
- e47c6c7a - vkd3d-shader/sm4: Use the instruction array interface in compile_dxbc_tpf().
- 3bab3879 - vkd3d-shader/spirv: Pass a parser pointer to spirv_compiler_generate_spirv().
Toggle commit listadded 49 commits
-
3bab3879...6b82ba94 - 41 commits from branch
wine:master
- 007f894b - vkd3d-shader/sm1: Store parsed instructions in an array.
- a9aaa59d - vkd3d-shader/sm4: Store parsed instructions in an array.
- e8cb9060 - vkd3d-shader: Initialise the instruction array in vkd3d_shader_parser_init().
- e9a2642d - vkd3d-shader/trace: Use the instruction array interface in vkd3d_dxbc_binary_to_text().
- 2559d622 - vkd3d-shader: Use the instruction array interface in scan_with_parser().
- 2d3f0518 - vkd3d-shader/glsl: Use the instruction array interface in vkd3d_glsl_generator_generate().
- 2a5ae0a8 - vkd3d-shader/sm4: Use the instruction array interface in compile_dxbc_tpf().
- d14f42be - vkd3d-shader/spirv: Pass a parser pointer to spirv_compiler_generate_spirv().
Toggle commit list-
3bab3879...6b82ba94 - 41 commits from branch