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

[Henri Verbeet]

On Tue, 15 Jun 2021 at 09:12, Conor McCarthy <cmccarthy at codeweavers.com> wrote:
> 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.
>
Is that really true? E.g., suppose you have a
vkd3d_shader_descriptor_binding like this:

    {.register_index = 2, .binding.count = 8, ...}

and then two vkd3d_shader_descriptor_range's like this:

    {.first = 3, .last = 5, ...}
    {.first = 6, .last = 8, ...}

The "binding_base_idx" for those would be 2 in either case. (And at
least in the current version of the patch, it's assigned
"current->register_index" in
vkd3d_dxbc_compiler_get_descriptor_binding() if it's not ~0u)

> > 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.
>
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.)

> >> + 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.

That should work. Alternatively though, it shouldn't be hard to fix
things up to match shader model 5.1 in the frontend, similar to what
we do with shader_sm4_map_resource_idx(), which would allow you to
just always use the shader model 5.1 path here in the backend.