June 15, 2021 12:39 AM, "Henri Verbeet" <hverbeet(a)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.