[v4 2/6] d3dx9: add preshaders to effect.

[Matteo Bruni]

2016-03-17 12:59 GMT+01:00 Paul Gofman <gofmanp at gmail.com>:

> +enum PRES_REG_TABLES
> +{
> +    PRES_REGTAB_NONE,
> +    PRES_REGTAB_IMMED, /* immediate double constants from CLIT */
> +    PRES_REGTAB_CONST,
> +    PRES_REGTAB_VERTEX, /* not used */
> +    PRES_REGTAB_OCONST,
> +    PRES_REGTAB_OBCONST,
> +    PRES_REGTAB_OICONST,
> +    PRES_REGTAB_REG,
> +    PRES_REGTAB_MAX,
> +    PRES_REGTAB_FIRST = PRES_REGTAB_IMMED,
> +    PRES_REGTAB_LAST = PRES_REGTAB_REG,
> +};

You can drop PRES_REGTAB_NONE unless it's going to be used.

> +++ b/dlls/d3dx9_36/preshader.c
> @@ -0,0 +1,627 @@
> +/*
> + * FX preshader parsing & execution, setting shader constants
> + *     from effect parameters.
> + *

It looks like that kind of general file description is omitted in
newer source files. Maybe drop it? I don't particularly care though.

> +    /* TODO: use double precision for x64 arch */
> +    {sizeof(float),  4, PRES_VT_FLOAT }  /* PRES_REGTAB_REG */

Nitpick, just mention "64 bit" or something, we don't want to make all
the non-Intel archs angry ;)

> +#define PRES_VS_BITS_PER_WORD (sizeof(unsigned int) * 8)

What does the _VS_ part of the name mean?
I'm also a bit thrown off by the _WORD part, mostly because in Win32
WORD is a 16 bit while DWORD is 32 bit and unsigned int is also 32
bit. Maybe it's just me, I don't know.

> +static void dump_shader_bytecode(void *data, unsigned int size)
> +{
> +    unsigned int *words = (unsigned int *)data;

Same here. I guess you could call it "bytecode" or such.

> +static unsigned int *find_bytecode_comment(unsigned int *ptr, unsigned int fourcc)
> +{
> +    while ((*ptr & 0xFFFF) == 0xFFFE)

A couple leftover uppercase hexadecimal constants.

> +    hr = D3DXGetShaderConstantTable(byte_code, &ctab);
> +    if (FAILED(hr) || !ctab)
> +    {
> +        TRACE("Could not get CTAB data, hr %#x.\n", hr);
> +        /* returning OK is not a typo */
> +        return D3D_OK;
> +    }

Sorry, back to that comment again. I guess it would be better to spell
out why that's the intended return value instead (e.g. "Not returning
an error, shaders without a CTAB are valid.").

> +static void dump_arg(struct d3dx_regstore *rs, struct d3dx_pres_operand *arg, int component_count)
> +{
> +    static const char *xyzw_str = "xyzw";
> +    unsigned int i, table;
> +
> +    table = arg->table;
> +    if (table == PRES_REGTAB_IMMED)
> +    {
> +        TRACE("(");
> +        for (i = 0; i < component_count; ++i)
> +            TRACE(i < component_count - 1 ? "%lf, " : "%lf",
> +                    ((double *)rs->tables[PRES_REGTAB_IMMED])[arg->offset + i]);

%lf is not right, %f already expects a double. I'd just use %.16e
(which has the added benefit of showing something different from 0 in
the "precision" test from the previous patch). Another option would be
to use the same format used by MS when disassembling shaders, although
this seems to be a case where it's worth diverging.

> +static void dump_ins(struct d3dx_regstore *rs, struct d3dx_pres_ins *ins)
> +{
> +    unsigned int i;
> +
> +    TRACE("    %s ", pres_op_info[ins->op].mnem);
> +    dump_arg(rs, &ins->output, pres_op_info[ins->op].func_all_comps ? 1 : ins->component_count);
> +    for (i = 0; i < pres_op_info[ins->op].input_count; ++i)
> +    {
> +        TRACE(",");

With a blank after the comma it would be prettier.

There is still some inconsistent formatting (e.g. "== NULL" vs "!",
uppercase hexadecimals, cnt vs count).

A possible idea to split this patch further: what if you move the
addition of (most of) the contents of the various functions in
preshader.c to a separate patch? That is, make a patch which adds the
various calls to d3dx_create_param_eval(), d3dx_free_param_eval() and
similar (i.e. most if not all the changes to effect.c) but only
introduces a skeleton implementation of those functions in
preshader.c. Those are actually fleshed out only in the next patch.
Many of the new preshader-related definitions can probably also go
into the second patch.
The traces of the disassembled preshader can also potentially go to a
separate patch (leaving only dump_shader_bytecode() in the initial
implementation - btw, I think it should be called
dump_preshader_bytecode() given the renaming of the other stuff).