[PATCH vkd3d v2 3/3] vkd3d-shader: Emit SPIR-V for descriptor arrays and arrayed bindings.

[Conor McCarthy]

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