June 15, 2021 12:39 AM, "Henri Verbeet" hverbeet@gmail.com wrote:
Do we need "binding_base_idx" to be part of vkd3d_symbol_register_data and vkd3d_symbol_resource_data? My first thought would be that that should be part of the vkd3d_array_variable structure, and vkd3d_symbol_resource_data should just point to that. Similar concerns probably apply to "contained_type_id" and "storage_class".
Multiple symbols can map to one array variable, and each can have a different base index. However, "contained_type_id" and "storage_class" can be stored in the array variable structure.
So this is essentially storing a vkd3d_shader_descriptor_binding structure and the corresponding SPIR-V variable, right? Should this perhaps be called something like "vkd3d_symbol_descriptor_binding" and be stored in the existing "symbol_table" tree?
Yes, I can declare a new symbol type and store it in "symbol_table".
Is the special case of "binding_base_idx == ~-0u" really needed? It seems users of "binding_base_idx" should be able to handle the actual value just fine.
The special case can be eliminated by always declaring an array even for single bindings. It means a little extra SPIR-V, and an extra entry in the symbol table, but accessing an array[1] at constant index 0 should ultimately compile to the same machine code as a scalar variable.
- if (shader_is_sm_5_1(compiler))
- {
- struct vkd3d_shader_register_index index = reg->idx[1];
- if (index.rel_addr)
- vkd3d_spirv_enable_descriptor_indexing(&compiler->spirv_builder);
- index.offset -= binding_base_idx;
- index_id = vkd3d_dxbc_compiler_emit_register_addressing(compiler, &index);
- }
- else
- {
- index_id = vkd3d_dxbc_compiler_get_constant_uint(compiler, reg->idx[0].offset -
binding_base_idx);
- }
- return index_id;
+}
We've been able to avoid version checks like this so far, and it's not immediately obvious to me why we need this one.
I can replace it with an index variable in the compiler struct for accessing reg->idx. That would be simpler and eliminate the branching.