[PATCH 1/2] d3dx9: Add 'exp' preshader opcode.

[Matteo Bruni]

2016-04-21 10:56 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
> @@ -43,6 +43,7 @@ enum pres_ops
>      PRESHADER_OP_SIN,
>      PRESHADER_OP_COS,
>      PRESHADER_OP_RSQ,
> +    PRESHADER_OP_EXP,
>  };

>  #define PRES_OPCODE_MASK 0x7ff00000
>  #define PRES_OPCODE_SHIFT 20
> @@ -119,6 +121,7 @@ static const struct op_info pres_op_info[] =
>      {0x108, "sin", 1, 0, pres_sin}, /* PRESHADER_OP_SIN */
>      {0x109, "cos", 1, 0, pres_cos}, /* PRESHADER_OP_COS */
>      {0x107, "rsq", 1, 0, pres_rsq}, /* PRESHADER_OP_RSQ */
> +    {0x105, "exp", 1, 0, pres_exp}, /* PRESHADER_OP_EXP */
>  };

It might make sense to keep the array sorted by opcode (which requires
either adding the enum into the table or, most likely, reordering the
enum too). That way you can binary search into it in the future, if
necessary.
No particular hurry for that though.

> +static void test_preshader_op(IDirect3DDevice9 *device, const char *op_mnem,
> +        unsigned int opcode, unsigned int args_count, unsigned int *expected_result,
> +        D3DXVECTOR4 *fvect1, D3DXVECTOR4 *fvect2, unsigned int ulps, BOOL *todo)
> +{
> +    DWORD test_effect_blob[ARRAY_SIZE(test_effect_preshader_effect_blob)];

It's not a huge array but it seems better to avoid putting it on the
stack nonetheless - i.e. please allocate it dynamically with
HeapAlloc().

> +    HRESULT hr;
> +    ID3DXEffect *effect;
> +    D3DLIGHT9 light;
> +    unsigned int i, npasses;

passes_count?

> +    float *v;
> +    unsigned int op_pos, op_step, result_index;
> +    D3DXHANDLE param;
> +
> +    if (args_count == 1)
> +    {
> +        op_pos = TEST_EFFECT_PRESHADER_OP0_POS;
> +        op_step = TEST_EFFECT_PRESHADER_OP0_INS_SIZE;
> +        result_index = 0;
> +    }
> +    else if (args_count == 2)
> +    {
> +        op_pos = TEST_EFFECT_PRESHADER_OPMUL_POS;
> +        op_step = TEST_EFFECT_PRESHADER_OPMUL_INS_SIZE;
> +        result_index = 2;
> +    }
> +    else
> +    {
> +        return;
> +    }

That doesn't seem particularly nice, especially WRT extending this
test later on.
It seems better to have the "original" bytecode and these variables
you compute here passed in from the caller (maybe stored in a struct
together with the opcode parameters).
test_effect_preshader_ops() should probably be table-based. At that
point the *_POS and *_SIZE defines might become unnecessary; if they
stay they should probably be named after the argument count (which is
the actually interesting part, the original operation is going to
change for the test and can be figured out from the light index)
instead.

> +    memcpy(test_effect_blob, test_effect_preshader_effect_blob, sizeof test_effect_blob);
> +    for (i = 0; i < 4; i++)

++i, for consistency.