Re: [PATCH] wined3d: Add #defines and logs for missing new SM5 modifiers.

[Józef Kucia]

This patch isn't very useful on its own. It just changes FIXME()
messages. It would be much better to save the decoded modifiers in
wined3d_shader_instruction and print them in the shader tracing
backend (see shader_trace_init() in shader.c). Other than that, the
patch has some coding style issues.

On Thu, Jun 2, 2016 at 3:49 PM, Guillaume Charifi
<guillaume.charifi(a)sfr.fr> wrote:
> Signed-off-by: Guillaume Charifi <guillaume.charifi(a)sfr.fr>
> ---
>  dlls/wined3d/shader_sm4.c | 78 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 54 insertions(+), 24 deletions(-)
>
> diff --git a/dlls/wined3d/shader_sm4.c b/dlls/wined3d/shader_sm4.c
> index eb8e4e5..3adaf70 100644
> --- a/dlls/wined3d/shader_sm4.c
> +++ b/dlls/wined3d/shader_sm4.c
> @@ -25,6 +25,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(d3d_shader);
>  WINE_DECLARE_DEBUG_CHANNEL(d3d_bytecode);
>
> #define WINED3D_SM4_INSTRUCTION_MODIFIER        (0x1u << 31)
> +#define WINED3D_SM4_MODIFIER_TYPE_MASK          0x3f
>
> #define WINED3D_SM4_MODIFIER_AOFFIMMI           0x1
>  #define WINED3D_SM4_AOFFIMMI_U_SHIFT            9
> @@ -34,6 +35,22 @@ WINE_DECLARE_DEBUG_CHANNEL(d3d_bytecode);
>  #define WINED3D_SM4_AOFFIMMI_W_SHIFT            17
>  #define WINED3D_SM4_AOFFIMMI_W_MASK             (0xfu << WINED3D_SM4_AOFFIMMI_W_SHIFT)
>
> +#define WINED3D_SM5_MODIFIER_RESDIM             0x2
> +#define WINED3D_SM5_RESDIM_RES_TYPE_SHIFT       6
> +#define WINED3D_SM5_RESDIM_RES_TYPE_MASK        (0x1fu << WINED3D_SM5_RESDIM_RES_TYPE_SHIFT)
> +#define WINED3D_SM5_RESDIM_BUF_STRIDE_SHIFT     11
> +#define WINED3D_SM5_RESDIM_BUF_STRIDE_MASK      (0xfffu << WINED3D_SM5_RESDIM_BUF_STRIDE_SHIFT)
> +
> +#define WINED3D_SM5_MODIFIER_RETTYPE            0x3
> +#define WINED3D_SM5_RETTYPE_X_SHIFT             6
> +#define WINED3D_SM5_RETTYPE_X_MASK              (0xfu << WINED3D_SM5_RETTYPE_X_SHIFT)
> +#define WINED3D_SM5_RETTYPE_Y_SHIFT             10
> +#define WINED3D_SM5_RETTYPE_Y_MASK              (0xfu << WINED3D_SM5_RETTYPE_Y_SHIFT)
> +#define WINED3D_SM5_RETTYPE_Z_SHIFT             14
> +#define WINED3D_SM5_RETTYPE_Z_MASK              (0xfu << WINED3D_SM5_RETTYPE_Z_SHIFT)
> +#define WINED3D_SM5_RETTYPE_W_SHIFT             18
> +#define WINED3D_SM5_RETTYPE_W_MASK              (0xfu << WINED3D_SM5_RETTYPE_W_SHIFT)
> +
>  #define WINED3D_SM4_INSTRUCTION_LENGTH_SHIFT    24
>  #define WINED3D_SM4_INSTRUCTION_LENGTH_MASK     (0x1fu << WINED3D_SM4_INSTRUCTION_LENGTH_SHIFT)
>
> @@ -1163,32 +1180,45 @@ static BOOL shader_sm4_read_dst_param(struct wined3d_sm4_data *priv, const DWORD
>
> static void shader_sm4_read_instruction_modifier(DWORD modifier, struct wined3d_shader_instruction *ins)
>  {
> -    static const DWORD recognized_bits = WINED3D_SM4_INSTRUCTION_MODIFIER
> -            | WINED3D_SM4_MODIFIER_AOFFIMMI
> -            | WINED3D_SM4_AOFFIMMI_U_MASK
> -            | WINED3D_SM4_AOFFIMMI_V_MASK
> -            | WINED3D_SM4_AOFFIMMI_W_MASK;
> -
> -    if (modifier & ~recognized_bits)
> +    switch (modifier & WINED3D_SM4_MODIFIER_TYPE_MASK)
>      {
> -        FIXME("Unhandled modifier 0x%08x.\n", modifier);
> -    }
> -    else
> -    {
> -        /* Bit fields are used for sign extension */
> -        struct
> +        case WINED3D_SM4_MODIFIER_AOFFIMMI:
>          {
> -            int u : 4;
> -            int v : 4;
> -            int w : 4;
> -        }
> -        aoffimmi;
> -        aoffimmi.u = (modifier & WINED3D_SM4_AOFFIMMI_U_MASK) >> WINED3D_SM4_AOFFIMMI_U_SHIFT;
> -        aoffimmi.v = (modifier & WINED3D_SM4_AOFFIMMI_V_MASK) >> WINED3D_SM4_AOFFIMMI_V_SHIFT;
> -        aoffimmi.w = (modifier & WINED3D_SM4_AOFFIMMI_W_MASK) >> WINED3D_SM4_AOFFIMMI_W_SHIFT;
> -        ins->texel_offset.u = aoffimmi.u;
> -        ins->texel_offset.v = aoffimmi.v;
> -        ins->texel_offset.w = aoffimmi.w;
> +            /* Bit fields are used for sign extension */
> +            struct {
> +                int u : 4;
> +                int v : 4;
> +                int w : 4;
> +            } aoffimmi;

Generally, in wined3d, curly braces should be put on their own line, i.e.
struct
{
...
}
aoffimmi;

The unnamed structs seem to be an exception. The following formatting
is acceptable as well:
struct
{
...
} aofimmi;
> +
> +            aoffimmi.u = (modifier & WINED3D_SM4_AOFFIMMI_U_MASK) >> WINED3D_SM4_AOFFIMMI_U_SHIFT;
> +            aoffimmi.v = (modifier & WINED3D_SM4_AOFFIMMI_V_MASK) >> WINED3D_SM4_AOFFIMMI_V_SHIFT;
> +            aoffimmi.w = (modifier & WINED3D_SM4_AOFFIMMI_W_MASK) >> WINED3D_SM4_AOFFIMMI_W_SHIFT;
> +            ins->texel_offset.u = aoffimmi.u;
> +            ins->texel_offset.v = aoffimmi.v;
> +            ins->texel_offset.w = aoffimmi.w;
> +            break;
> +        };

Please remove stray ";".
> +
> +        case WINED3D_SM5_MODIFIER_RESDIM:
> +            FIXME("Unhandled modifier 0x%08x (WINED3D_SM5_MODIFIER_RESDIM, res_type=0x%02x).\n",
> +                    modifier,
> +                    (modifier & WINED3D_SM5_RESDIM_RES_TYPE_MASK) >> WINED3D_SM5_RESDIM_RES_TYPE_SHIFT
> +            );

Please avoid leaving ");" on its own line.
> +            break;
> +
> +        case WINED3D_SM5_MODIFIER_RETTYPE:
> +            FIXME("Unhandled modifier 0x%08x (WINED3D_SM5_MODIFIER_RETTYPE, x=0x%01x, y=0x%01x, z=0x%01x, w=0x%01x).\n",
> +                    modifier,
> +                    (modifier & WINED3D_SM5_RETTYPE_X_MASK) >> WINED3D_SM5_RETTYPE_X_SHIFT,
> +                    (modifier & WINED3D_SM5_RETTYPE_Y_MASK) >> WINED3D_SM5_RETTYPE_Y_SHIFT,
> +                    (modifier & WINED3D_SM5_RETTYPE_Z_MASK) >> WINED3D_SM5_RETTYPE_Z_SHIFT,
> +                    (modifier & WINED3D_SM5_RETTYPE_W_MASK) >> WINED3D_SM5_RETTYPE_W_SHIFT
> +            );
> +            break;
> +
> +        default:
> +            FIXME("Unhandled modifier 0x%08x.\n", modifier);
>      }
>  }
>
> --
> 2.7.4

Thanks for working on this.