May 2, 2023
1:39 p.m.
On Tue May 2 13:39:36 2023 +0000, Henri Verbeet wrote: > > > This works, but only because sizeof(\*signature->elements) is > smaller than sizeof(\*src->elements). > > > > I changed it to `vkd3d_calloc()`, but I see no size mismatch here. > struct sm6_signature_element has the extra "register_count" field that > struct vkd3d_shader_signature_element doesn't have; that was the point > of this patch, of course. So if "element_count * sizeof(struct > sm6_signature_element)" didn't overflow, "element_count * sizeof(struct > vkd3d_shader_signature_element) wouldn't either. Though I gather from > your reply that that was not the reasoning behind using vkd3d_malloc(). :) > I think the first 5 patches in the series are fine, and could go in as a > separate series. > Patch 7/7 doesn't touch spirv.c and glsl.c, and I think it should. I.e., > spirv_compiler_generate_spirv() and vkd3d_glsl_generator_generate() > would ideally use the instruction's "source_index" to set the location. > Those parts could do go in independently of the changes in patch 6/7. > Some comments on patch 6/7: > - Most (all?) of the VKD3D_SHADER_ERROR_IR_* errors seem like things > that the frontend could/should catch. Doing them in the frontend would > also allow introducing that validation as separate patches. We could > possibly introduce a separate validation pass for that in tpf.c, but I > don't think we strictly have to. > - There's a fair amount of shared state introduced to struct > vkd3d_shader_normaliser, but most of it seems local to a particular > transformation pass. It probably doesn't need to be part of the > vkd3d_shader_normaliser structure, and it probably doesn't even need to > be visible outside of ir.c. To some extent that's true for some of the > existing shared state in struct vkd3d_shader_normaliser as well. > - I think we should add the different signatures to struct > vkd3d_shader_instruction_array instead of struct > vkd3d_shader_normaliser. It would probably simplify passing around the > signatures in a couple of other places as well. (In particular, places > that currently either use struct vkd3d_shader_desc or pass signatures > around as separate parameters.) > - I don't think the use of the ternary operator in > signature_element_mask_compare() improves readability. > - Somewhat similarly, the "if ((array_size = (input->register_count > > 1) ? input->register_count : 0))" line in > spirv_compiler_emit_default_control_point_phase() seems a bit too > clever. I'd suggest doing something like the following: > ```c > if ((array_size = input->register_count) > 1) > type_id = vkd3d_spirv_get_op_type_array(...); > else > array_size = 0; > ``` > - The patch introduces "outer_array_length" parameters to > spirv_compiler_emit_array_variable() and > spirv_compiler_emit_builtin_variable(), but the conventional way we'd do > that would be to make "array_length"/"array_size" and array and add > something like an "index_count"/"length_count"/"size_count" parameter. > For example, like we do with vkd3d_spirv_build_op_access_chain() and > vkd3d_spirv_build_op_access_chain1(). These changes could be split out > as a separate patch. (E.g. "Support emitting multi-dimensional array > variables in spirv_compiler_emit_array_variable().") > - I'm tempted to suggest adding an "index_count" field to struct > vkd3d_shader_register. (As opposed to comparing the "offset" field of > register indices with ~0u.) The original used `sizeof(*signature->elements)` to allocate signature->elements, not `sizeof(*src->elements)`, but if everyone is happy with it now then no worries :) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/181#note_31739