Re: [PATCH vkd3d 2/5] vkd3d-shader: Introduce a descriptor array symbol type to struct vkd3d_symbol.

[Henri Verbeet]

On Fri, 9 Jul 2021 at 07:47, Conor McCarthy  wrote:
> +static uint32_t vkd3d_dxbc_compiler_get_resource_index(struct vkd3d_dxbc_compiler *compiler,
> -
>    const struct vkd3d_shader_register *reg, unsigned int binding_base_idx,
> -
>    enum vkd3d_shader_resource_type resource_type)
>
> +{
> - struct vkd3d_shader_register_index index = reg->idx[1];
> - uint32_t index_id;
> -
> - if (index.rel_addr)
> - {
> -
>    FIXME("Descriptor dynamic indexing is not supported.\n");
> -
>    vkd3d_dxbc_compiler_error(compiler, VKD3D_SHADER_ERROR_SPV_DESCRIPTOR_IDX_UNSUPPORTED,
> -
>            "Cannot dynamically index a descriptor array of type %#x, id %u. "
> -
>            "Dynamic indexing is not supported.", reg->type, reg->idx[0].offset);
> - }
> -
> - index.offset -= binding_base_idx;
> - index_id = vkd3d_dxbc_compiler_emit_register_addressing(compiler, &index);
> -
> - return index_id;
>
> +}
> -

"resource_type" is unused here.
Would it make sense to name this
vkd3d_dxbc_compiler_get_descriptor_index() instead?
> +static uint32_t vkd3d_dxbc_compiler_get_resource_variable(struct vkd3d_dxbc_compiler *compiler,
> -
>    const struct vkd3d_symbol_descriptor_array_data *array_data, uint32_t ptr_type_id,
> -
>    const struct vkd3d_shader_register *reg, const struct vkd3d_shader_descriptor_binding *binding,
> -
>    const struct vkd3d_symbol **array_symbol)
>
> +{
> - struct vkd3d_spirv_builder *builder = &compiler->spirv_builder;
> -
> - if (array_data->contained_type_id)
> - {
> -
>    struct vkd3d_symbol *array_symbol_entry;
> -
>    struct vkd3d_symbol symbol;
> -
>    struct rb_entry *entry;
> -
> -
>    /* Declare one array variable per Vulkan binding, and use it for all array declarations
> -
>     * which map to it. In this case ptr_type_id must point to an array type. */
> -
>    symbol.type = VKD3D_SYMBOL_DESCRIPTOR_ARRAY;
> -
>    memset(&symbol.key, 0, sizeof(symbol.key));
> -
>    symbol.key.descriptor_array.ptr_type_id = ptr_type_id;
> -
>    symbol.key.descriptor_array.set = binding->set;
> -
>    symbol.key.descriptor_array.binding = binding->binding;
> -
>    if ((entry = rb_get(&compiler->symbol_table, &symbol)))
> -
>    {
> -
>        array_symbol_entry = RB_ENTRY_VALUE(entry, struct vkd3d_symbol, entry);
> -
>        *array_symbol = array_symbol_entry;
> -
>        return array_symbol_entry->id;
> -
>    }
> -
> -
>    symbol.id = vkd3d_spirv_build_op_variable(builder, &builder->global_stream,
> -
>        ptr_type_id, array_data->storage_class, 0);
> -
>    symbol.descriptor_array = NULL;
> -
>    vkd3d_dxbc_compiler_emit_descriptor_binding(compiler, symbol.id, binding);
> -
>    vkd3d_dxbc_compiler_emit_register_debug_name(builder, symbol.id, reg);
> -
> -
>    symbol.info.descriptor_array = *array_data;
> -
>    *array_symbol = vkd3d_dxbc_compiler_put_symbol(compiler, &symbol);
> -
> -
>    return symbol.id;
> - }
> - else
> - {
> -
>    uint32_t var_id = vkd3d_spirv_build_op_variable(builder, &builder->global_stream,
> -
>            ptr_type_id, array_data->storage_class, 0);
> -
>    vkd3d_dxbc_compiler_emit_descriptor_binding(compiler, var_id, binding);
> -
>    vkd3d_dxbc_compiler_emit_register_debug_name(builder, var_id, reg);
> -
>    *array_symbol = NULL;
> -
>    return var_id;
> - }
>
> +}
> -

It seems nicer to handle the non-arrayed case first, and then save an
indentation level on the arrayed case. I.e.:

static uint32_t vkd3d_dxbc_compiler_get_resource_variable(...)
    {
       ...
       if (!array_data->contained_type_id)
       {
           ...
           return var_id;
       }

...
       return symbol.id;
    }
> +static uint32_t vkd3d_dxbc_compiler_build_resource_variable(struct vkd3d_dxbc_compiler *compiler,
> -
>    SpvStorageClass storage_class, uint32_t type_id, const struct vkd3d_shader_register *reg,
> -
>    const struct vkd3d_shader_descriptor_binding *binding, unsigned int binding_base_idx,
> -
>    bool array_variable, const struct vkd3d_symbol **array_symbol)
>
> +{
> - struct vkd3d_spirv_builder *builder = &compiler->spirv_builder;
> - struct vkd3d_symbol_descriptor_array_data array_data;
> - uint32_t ptr_type_id;
> -
> - array_data.storage_class = storage_class;
> - array_data.contained_type_id = 0;
> - array_data.binding_base_idx = binding_base_idx;
> - if (array_variable)
> - {
> -
>    array_data.contained_type_id = type_id;
> -
>    type_id = vkd3d_spirv_get_op_type_array(builder, type_id,
> -
>            vkd3d_dxbc_compiler_get_constant_uint(compiler, binding->count));
> - }
> - ptr_type_id = vkd3d_spirv_get_op_type_pointer(builder, array_data.storage_class, type_id);
> -
> - return vkd3d_dxbc_compiler_get_resource_variable(compiler, &array_data, ptr_type_id, reg, binding, array_symbol);
>
> +}
> -

Would it make sense to name this
vkd3d_dxbc_compiler_build_descriptor_variable() instead?
I think we can just merge vkd3d_dxbc_compiler_get_resource_variable()
into this function, which would then allow for some simplifications of
the combined function.
> static void vkd3d_dxbc_compiler_emit_dcl_constant_buffer(struct vkd3d_dxbc_compiler *compiler,
>          const struct vkd3d_shader_instruction *instruction)
>  {
> - uint32_t vec4_id, array_type_id, length_id, struct_id, pointer_type_id, var_id;
> const struct vkd3d_shader_constant_buffer *cb = &instruction->declaration.cb;
> struct vkd3d_spirv_builder *builder = &compiler->spirv_builder;
> -
>
> uint32_t vec4_id, array_type_id, length_id, struct_id, var_id;
> const SpvStorageClass storage_class = SpvStorageClassUniform;
> const struct vkd3d_shader_register *reg = &cb->src.reg;
> struct vkd3d_push_constant_buffer_binding *push_cb;
> -
>
> struct vkd3d_shader_descriptor_binding binding;
> -
>
> const struct vkd3d_symbol *array_symbol;
> struct vkd3d_symbol reg_symbol;
> -
>
> unsigned int binding_base_idx;
>
> assert(!(instruction->flags & ~VKD3DSI_INDEXED_DYNAMIC));
>
> @@ -5321,18 +5448,16 @@ static void vkd3d_dxbc_compiler_emit_dcl_constant_buffer(struct vkd3d_dxbc_compi
>      vkd3d_spirv_build_op_member_decorate1(builder, struct_id, 0, SpvDecorationOffset, 0);
>      vkd3d_spirv_build_op_name(builder, struct_id, "cb%u_struct", cb->size);
> - pointer_type_id = vkd3d_spirv_get_op_type_pointer(builder, storage_class, struct_id);
> - var_id = vkd3d_spirv_build_op_variable(builder, &builder->global_stream,
> -
>        pointer_type_id, storage_class, 0);
> -
> - vkd3d_dxbc_compiler_emit_descriptor_binding_for_reg(compiler,
> -
>        var_id, reg, &cb->range, VKD3D_SHADER_RESOURCE_BUFFER, false);
> - binding = vkd3d_dxbc_compiler_get_descriptor_binding(compiler, reg, &cb->range,
> -
>        VKD3D_SHADER_RESOURCE_BUFFER, false, &binding_base_idx);
>
> - vkd3d_dxbc_compiler_emit_register_debug_name(builder, var_id, reg);
> -
>
> var_id = vkd3d_dxbc_compiler_build_resource_variable(compiler, storage_class, struct_id,
> -
>        reg, &binding, binding_base_idx, binding.count != 1 || cb->range.last == ~0u, &array_symbol);
>
> vkd3d_symbol_make_register(&reg_symbol, reg);
>    vkd3d_symbol_set_register_info(&reg_symbol, var_id, storage_class,
>            VKD3D_SHADER_COMPONENT_FLOAT, VKD3DSP_WRITEMASK_ALL);
> -
>
> reg_symbol.descriptor_array = array_symbol;
> vkd3d_dxbc_compiler_put_symbol(compiler, &reg_symbol);
>
> }

Somewhat similarly, if we passed a vkd3d_shader_register_range
structure and resource type to
vkd3d_dxbc_compiler_build_resource_variable(), we could move the
vkd3d_dxbc_compiler_get_descriptor_binding() call into
vkd3d_dxbc_compiler_build_resource_variable(), and then drop the
"binding", "binding_base_idx", and "array_variable" parameters.