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

[Józef Kucia]

On Fri, Jul 15, 2016 at 1:53 PM, Guillaume Charifi
 wrote:
> Signed-off-by: Guillaume Charifi
>
> 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);
}