From: "Conor McCarthy" Subject: Re: [PATCH vkd3d v2 3/3] vkd3d-shader: Emit SPIR-V for descriptor arrays and arrayed bindings. Message-Id: <91ad271c716d35e3632754a66772fc73@codeweavers.com> Date: Tue, 15 Jun 2021 07:12:32 +0000 In-Reply-To: References: <20210603050124.45254-1-cmccarthy@codeweavers.com> <20210603050124.45254-3-cmccarthy@codeweavers.com> June 15, 2021 12:39 AM, "Henri Verbeet" 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.