[v7 2/7] d3dx9: Implement preshader parsing.

[Matteo Bruni]

2016-03-28 9:20 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
> Signed-off-by: Paul Gofman <gofmanp at gmail.com>
> ---
> Changes
> - Changed opcode function to take double arguments and return double

Good catch.

> +static struct op_info pres_op_info[] =
> +{
> +    {0x000, "nop", 0, 0, NULL    }, /* PRESHADER_OP_NOP */
> +    {0x100, "mov", 1, 0, pres_mov}, /* PRESHADER_OP_MOV */
> +};

I think this can be const.

> +struct d3dx_pres_ins
> +{
> +    enum pres_ops op;
> +    /* first input argument is scalar,
> +       scalar component is propagated */
> +    BOOL scalar_op;
> +    unsigned int component_count;
> +    struct d3dx_pres_operand inputs[3];

The maximum number of input operands could use a #define. Not a huge
deal though.

> +static void regstore_free_tables(struct d3dx_regstore *rs)
> +{
> +    unsigned int i;
> +
> +    for (i = PRES_REGTAB_FIRST; i <= PRES_REGTAB_LAST; ++i)
> +    {
> +        if (rs->tables[i])
> +            HeapFree(GetProcessHeap(), 0, rs->tables[i]);
> +        if (rs->table_value_set[i])
> +            HeapFree(GetProcessHeap(), 0, rs->table_value_set[i]);
> +    }
> +}

You don't need to check for non-NULL, HeapFree() handles NULL
arguments just fine.

> +static unsigned int *find_bytecode_comment(unsigned int *ptr, unsigned int fourcc)
> +{
> +    while ((*ptr & 0xffff) == 0xfffe)
> +    {
> +        if (*(ptr + 1) == fourcc)
> +            return ptr + 2;
> +        ptr += 1 + (*ptr >> 16);
> +    }
> +    return NULL;
> +}

This is probably paranoid but in theory a broken or malicious effect
could send you rummaging outside of the bytecode buffer so all the
offsets should be checked. I don't think it's critical in practice
i.e. I'll probably sleep just fine if you decide to ignore this
issue...

> +static unsigned int *parse_pres_arg(unsigned int *ptr, struct d3dx_pres_operand *opr)
> +{
> +    static const enum pres_reg_tables reg_table[8] =
> +    {
> +        PRES_REGTAB_COUNT, PRES_REGTAB_IMMED, PRES_REGTAB_CONST, PRES_REGTAB_COUNT,
> +        PRES_REGTAB_OCONST, PRES_REGTAB_OBCONST, PRES_REGTAB_OICONST, PRES_REGTAB_REG

Matter of taste, so feel free to ignore this, but PRES_REGTAB_REG
sounds a bit vague to me. I'd go with something like PRES_REGTAB_TEMP.

> +    if (input_count != pres_op_info[i].input_count)
> +    {
> +        FIXME("Actual input args %u, expected %u, instruction %s.\n", input_count,
> +                pres_op_info[i].input_count, pres_op_info[i].mnem);
> +        return NULL;
> +    }
> +    for (i = 0; i < input_count; ++i)
> +    {
> +        ptr = parse_pres_arg(ptr, &ins->inputs[i]);
> +        if (!ptr)
> +            return NULL;
> +    }

Another pretty paranoid potential check, here you could verify that
input_count is < ARRAY_SIZE(ins->inputs). Of course that could only
fail if we encounter some instruction with more than 3 components AND
we update pres_op_info BUT we forget to update struct d3dx_pres_ins,
but I don't think it would hurt to be safe...

> +    for (i = 0; i < pres->ins_count; ++i)
> +    {
> +        unsigned int table;
> +
> +        for (j = 0; j < pres_op_info[pres->ins[i].op].input_count; ++j)
> +        {
> +            table = pres->ins[i].inputs[j].table;
> +            max_offset = get_reg_offset(table, pres->ins[i].inputs[j].offset + pres->ins[i].component_count - 1);
> +            pres->regs.table_sizes[table] = max(pres->regs.table_sizes[table], max_offset + 1);
> +        }
> +        table = pres->ins[i].output.table;
> +        max_offset = get_reg_offset(table, pres->ins[i].output.offset + pres->ins[i].component_count - 1);
> +        pres->regs.table_sizes[table] = max(pres->regs.table_sizes[table], max_offset + 1);
> +    }
> +    update_table_sizes_consts(pres->regs.table_sizes, &pres->inputs);

It probably makes sense to add a small update_table_size() helper (or
similar), the "same" 3 lines are repeated in the loop for the input
operands, for the output operand and in update_table_sizes_consts().

> +    for (i = PRES_REGTAB_IMMED + 1; i <= PRES_REGTAB_LAST; ++i)

It depends on what is coming up next but maybe it would make sense to
add an enum for "PRES_REGTAB_IMMED + 1" (incidentally,
PRES_REGTAB_FIRST is never used in this patch series).

> +    {
> +        TRACE("table_sizes[%u] %u.\n", i, peval->pres.regs.table_sizes[i]);

That trace looks a bit like a debugging leftover.

> +        if (FAILED(regstore_alloc_table(&peval->pres.regs, i)))
> +        {
> +            hr = E_OUTOFMEMORY;
> +            goto err_out;
> +        }
> +    }
> +
> +    if (TRACE_ON(d3dx))
> +        dump_bytecode(byte_code, byte_code_size);
> +
> +    *peval_out = peval;
> +    TRACE("retval %u, *peval_out %p.\n", D3D_OK, *peval_out);

Same with this. This one in particular doesn't seem worth keeping.

> +    return D3D_OK;
> +
> +err_out:
> +    FIXME("Error creating parameter evaluator.\n");
> +    d3dx_free_param_eval(peval);
>      *peval_out = NULL;
> -    return E_NOTIMPL;
> +    TRACE("retval %u, *peval_out %p.\n", hr, *peval_out);
> +    return hr;
> +}

You could print the HRESULT in the FIXME (e.g. FIXME("Error creating
parameter evaluator, returning %#x.\n", hr)). Tracing the value of
*peval_out doesn't seem particularly worthwhile.

> +static void d3dx_free_const_tab(struct d3dx_const_tab *ctab)
> +{
> +    if (ctab->inputs)
> +        HeapFree(GetProcessHeap(), 0, ctab->inputs);
> +    if (ctab->inputs_param)
> +        HeapFree(GetProcessHeap(), 0, ctab->inputs_param);
> +    if (ctab->ctab)
> +        ID3DXConstantTable_Release(ctab->ctab);
> +}
> +
> +static void d3dx_free_preshader(struct d3dx_preshader *pres)
> +{
> +    if (pres->ins)
> +        HeapFree(GetProcessHeap(), 0, pres->ins);
> +
> +    regstore_free_tables(&pres->regs);
> +    d3dx_free_const_tab(&pres->inputs);
>  }

You can drop all those ifs (I think there are a few more of them in
the middle of the patch I haven't specifically mentioned).