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
 wrote:
> Signed-off-by: Guillaume Charifi
>
> 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.