[PATCH 1/2] wined3d: Implement SM5 bfi opcode in glsl.

[Józef Kucia]

On Fri, Jul 15, 2016 at 1:53 PM, Guillaume Charifi
<guillaume.charifi at sfr.fr> wrote:
> Signed-off-by: Guillaume Charifi <guillaume.charifi at sfr.fr>
> ---
>  dlls/wined3d/glsl_shader.c | 72 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c
> index d8edfdc..b0d3a91 100644
> --- a/dlls/wined3d/glsl_shader.c
> +++ b/dlls/wined3d/glsl_shader.c
> @@ -4380,6 +4380,76 @@ static void shader_glsl_f32tof16(const struct wined3d_shader_instruction *ins)
>  #undef NB_COMPS
>  }

This patch requires 124405 so it should be in the same series.

>
> +
> +static void shader_glsl_bitfield_op(const struct wined3d_shader_instruction *ins)
> +{
> +#define NB_COMPS 4
> +    const DWORD write_masks[NB_COMPS] = {
> +        WINED3DSP_WRITEMASK_0,
> +        WINED3DSP_WRITEMASK_1,
> +        WINED3DSP_WRITEMASK_2,
> +        WINED3DSP_WRITEMASK_3,
> +    };

You shouldn't need this array. You can assume that
WINED3DSP_WRITE_MASK_i = WINED3DSP_WRITE_MASK_0 << i.

> +    const char dst_masks[NB_COMPS] = { 'x', 'y', 'z', 'w' };
> +    const char *prefix;
> +    const char *suffix;
> +    int nb_params;

For consistency, this should be named "param_count" and should be unsigned int.

> +    struct glsl_src_param src_param;
> +    DWORD write_mask;
> +    char dst_mask[6];
> +    char *dst_mask_ptr = dst_mask;
> +
> +    switch(ins->handler_idx)
> +    {
> +        case WINED3DSIH_BFI:
> +            prefix = "bitfieldInsert(";
> +            suffix = ")";
> +            nb_params = 4;
> +            break;
> +
> +        default:
> +            prefix = "<unhandled opcode>";
> +            suffix = "";
> +            nb_params = 0;
> +            FIXME("Can't handle opcode %s.\n", debug_d3dshaderinstructionhandler(ins->handler_idx));
> +    }
> +
> +    write_mask = shader_glsl_append_dst(ins->ctx->buffer, ins);
> +
> +    *dst_mask_ptr++ = '.';
> +    shader_addline(ins->ctx->buffer, "uvec4(\n");
> +
> +    for (int i = 0; i < NB_COMPS; i++)

This requires C99.

> +    {
> +        if (write_mask & write_masks[i])
> +        {
> +            shader_addline(ins->ctx->buffer, "%s", prefix);
> +            for(int j = nb_params - 1; j >= 0; j--)
> +            {
> +                shader_glsl_add_src_param(ins, &ins->src[j], write_masks[i], &src_param);
> +                if(j <= 1)
> +                    shader_addline(ins->ctx->buffer, "int(%s)", src_param.param_str);

You shouldn't need this cast. It's better to adjust the parameter
types and let shader_glsl_add_src_param() handle this.

> +                else
> +                    shader_addline(ins->ctx->buffer, "%s", src_param.param_str);
> +
> +                if(j)
> +                    shader_addline(ins->ctx->buffer, ", ");
> +            }
> +            shader_addline(ins->ctx->buffer, "%s", suffix);
> +            *dst_mask_ptr++ = dst_masks[i];
> +        } else
> +            shader_addline(ins->ctx->buffer, "0");
> +
> +        if(i != NB_COMPS - 1)
> +            shader_addline(ins->ctx->buffer, ",");
> +        shader_addline(ins->ctx->buffer, "\n");
> +    }
> +
> +    *dst_mask_ptr++ = '\0';

You shouldn't need to build dst_mask manually.

> +    shader_addline(ins->ctx->buffer, ")%s);\n", dst_mask);
> +#undef NB_COMPS

I don't like this duplicated enums.

Generally, this implementation seems overly complicated. You should be
able to do something similar to:

for (i = 0; i < 4; ++i)
{
  write_mask = WINED3DSP_WRITEMASK_0 << i;
  dst.write_mask = ins->dst[0].write_mask & write_mask;

  if (!(write_mask = shader_glsl_append_dst_ext(ins->ctx->buffer, ins,
&dst, dst.reg.data_type)))
    continue;

  shader_glsl_add_src_param(ins, &ins->src[0], write_mask, &bits);
  shader_glsl_add_src_param(ins, &ins->src[1], write_mask, &offset);
  shader_glsl_add_src_param(ins, &ins->src[2], write_mask, &insert);
  shader_glsl_add_src_param(ins, &ins->src[3], write_mask, &base);

  shader_addline(ins->ctx->buffer, "bitfieldInsert(%s, %s, %s, %s));\n",
    base.param_str, insert.param_str, offset.param_str, bits.param_str);
}