Re: [PATCH vkd3d v2 2/3] vkd3d-shader: Parse SM5.1 register ranges.

[Henri Verbeet]

On Thu, 3 Jun 2021 at 07:02, Conor McCarthy  wrote:
> -static unsigned int shader_sm4_map_resource_idx(struct vkd3d_shader_register *reg, const struct vkd3d_sm4_data *priv)
> +static void shader_sm4_read_register_indices(struct vkd3d_shader_resource *resource,
> -
>    struct vkd3d_shader_instruction *ins, struct vkd3d_sm4_data *priv)
>
> {
>      if (shader_is_sm_5_1(priv))
> -
>    return reg->idx[1].offset;
> - {
> -
>    resource->register_first = resource->reg.reg.idx[1].offset;
> -
>    resource->register_last = resource->reg.reg.idx[2].offset;
> -
>    if (resource->register_last < resource->register_first)
> -
>    {
> -
>        FIXME("Invalid register range [%u:%u].\n", resource->register_first, resource->register_last);
> -
>        ins->handler_idx = VKD3DSIH_INVALID;
> -
>    }
> - }
> else
> -
>    return reg->idx[0].offset;
> - {
> -
>    resource->register_first = resource->reg.reg.idx[0].offset;
> -
>    resource->register_last = resource->register_first;
> - }
>
> }

I don't think shader_sm4_read_register_indices() is a particularly
great name for what this does. In particular, actual register indices
(i.e., the "idx" field in the vkd3d_shader_register structure) are
read as part of shader_sm4_read_param(), and this resolves
descriptor/resource array ranges instead.

Error reporting should happen through vkd3d_shader_verror() instead,
so that we can report a proper compiler message to the application.
(See also e.g. hlsl_error(), preproc_error(),
vkd3d_dxbc_compiler_error(), etc. for examples.) We do of course have
existing code in the SM4 parser that still uses VKD3DSIH_INVALID
and/or FIXMEs/ERRs; it would be nice to get those cleaned up as well.
> static void shader_sm4_read_dcl_resource(struct vkd3d_shader_instruction *ins,
> @@ -635,6 +647,7 @@ static void shader_sm4_read_dcl_resource(struct vkd3d_shader_instruction *ins,
>          struct vkd3d_sm4_data *priv)
>  {
>      struct vkd3d_shader_semantic *semantic = &ins->declaration.semantic;
> - struct vkd3d_shader_resource *resource = &semantic->resource;
> enum vkd3d_sm4_resource_type resource_type;
> const DWORD *end = &tokens[token_count];
> enum vkd3d_sm4_data_type data_type;
>
> @@ -653,8 +666,7 @@ static void shader_sm4_read_dcl_resource(struct vkd3d_shader_instruction *ins,
>          semantic->resource_type = resource_type_table[resource_type];
>      }
>      reg_data_type = opcode == VKD3D_SM4_OP_DCL_RESOURCE ? VKD3D_DATA_RESOURCE : VKD3D_DATA_UAV;
> - shader_sm4_read_dst_param(priv, &tokens, end, reg_data_type, &semantic->resource.reg);
> - semantic->resource.register_index = shader_sm4_map_resource_idx(&semantic->resource.reg.reg, priv);
> -
>
> shader_sm4_read_dst_param(priv, &tokens, end, reg_data_type, &resource->reg);
>
> components = *tokens++;
> for (i = 0; i < VKD3D_VEC4_SIZE; i++)
>
> @@ -675,23 +687,21 @@ static void shader_sm4_read_dcl_resource(struct vkd3d_shader_instruction *ins,
>      if (reg_data_type == VKD3D_DATA_UAV)
>          ins->flags = (opcode_token & VKD3D_SM5_UAV_FLAGS_MASK) >> VKD3D_SM5_UAV_FLAGS_SHIFT;
> - shader_sm4_read_register_space(priv, &tokens, end, &semantic->resource.register_space);
> - shader_sm4_read_register_space(priv, &tokens, end, &resource->register_space);
> - shader_sm4_read_register_indices(resource, ins, priv);
>
> }

An easy way to split this patch would be to introduce the changes for
each instruction as a separate patch.
> @@ -640,7 +640,7 @@ struct vkd3d_shader_resource
>  {
>      struct vkd3d_shader_dst_param reg;
>      unsigned int register_space;
> - unsigned int register_index;
> - unsigned int register_first, register_last;
>
> };
>
> enum vkd3d_decl_usage
> @@ -715,14 +715,16 @@ struct vkd3d_shader_register_semantic
>  struct vkd3d_shader_sampler
>  {
>      struct vkd3d_shader_src_param src;
> - unsigned int register_space, register_index;
> - unsigned int register_space;
> - unsigned int register_first, register_last;
>
> };
>
> struct vkd3d_shader_constant_buffer
>  {
>      struct vkd3d_shader_src_param src;
>      unsigned int size;
> - unsigned int register_space, register_index;
> - unsigned int register_space;
> - unsigned int register_first, register_last;
>
> };

This will fail to compile; there are references to "register_index" in
e.g. spirv.c.
In any case, it seems like it may be helpful to introduce a structure like this:

struct vkd3d_shader_descriptor_range
    {
        unsigned int space;
        unsigned int first, last;
    };

You could then do something like the following instead of
shader_sm4_read_register():

static void shader_sm4_resolve_descriptor_range(struct vkd3d_sm4_data *priv,
            const struct vkd3d_shader_register *reg, struct
vkd3d_shader_descriptor_range *range)
    {
        [...]
    }

and also use that structure for e.g. vkd3d_shader_scan_add_descriptor().

It would also be nice to make the corresponding changes to trace.c as
well, so that descriptor ranges are also properly disassembled.