From: Henri Verbeet Subject: Re: [PATCH vkd3d v2 3/3] vkd3d-shader: Emit SPIR-V for descriptor arrays and arrayed bindings. Message-Id: Date: Tue, 15 Jun 2021 16:03:56 +0200 In-Reply-To: <3cd7a066edd9c35a38b50e60da6f03cf@codeweavers.com> References: <20210603050124.45254-1-cmccarthy@codeweavers.com> <20210603050124.45254-3-cmccarthy@codeweavers.com> <91ad271c716d35e3632754a66772fc73@codeweavers.com> <3cd7a066edd9c35a38b50e60da6f03cf@codeweavers.com> On Tue, 15 Jun 2021 at 14:55, Conor McCarthy wrote: > June 15, 2021 8:36 PM, "Henri Verbeet" wrote: > > That's one possibility, but it would also be possible to just compare > > the "count" field from the vkd3d_shader_descriptor_binding structure > > against 1, instead of comparing "binding_base_idx" against ~0u. (I.e., > > when storing a vkd3d_shader_descriptor_binding structure in the > > vkd3d_array_variable/vkd3d_symbol_descriptor_binding structure.) > > The one potential problem I see is it's possible to declare in a shader an unbounded resource array which maps to a range containing only one descriptor, and then dynamically index it with a variable that's always zero. Unlikely to occur, but the dynamic indexing would fail because with binding.count == 1, a scalar variable is declared. That's a good point, although also somewhat orthogonal; in the current version of the patch vkd3d_dxbc_compiler_get_resource_variable() uses the "array_variable" parameter to decide whether to create a scalar or array variable. Its callers all use some variant of "binding_base_idx != ~0u || register_last == ~0u" to set that, which could be trivially replaced with something like "binding.count > 1 || register_last == ~0u", because vkd3d_dxbc_compiler_get_resource_variable() also takes "binding" as an argument. It could be argued that callers should pass a vkd3d_shader_descriptor_range structure to vkd3d_dxbc_compiler_get_resource_variable() instead of the "array_variable" parameter, in which case vkd3d_dxbc_compiler_get_resource_variable() would be able to resolve whether or not to create an array variable for itself, using something like "binding->count > 1 || range->last == ~0u". There's certainly something to be said for the simplicity of always creating array variables though.