From: Henri Verbeet <hverbeet@gmail.com>
Subject: Re: [PATCH vkd3d v2 2/3] vkd3d-shader: Parse SM5.1 register ranges.
Message-Id: <CAOsNvwxU38k2KTx+M2gpot_PtTq_fP2R0-XQbdCdGMsqJnLaYQ@mail.gmail.com>
Date: Mon, 14 Jun 2021 16:39:24 +0200
In-Reply-To: <20210603050124.45254-2-cmccarthy@codeweavers.com>
References: <20210603050124.45254-1-cmccarthy@codeweavers.com> <20210603050124.45254-2-cmccarthy@codeweavers.com>

On Thu, 3 Jun 2021 at 07:02, Conor McCarthy <cmccarthy@codeweavers.com> 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.