Re: [PATCH 2/3] d3dx9: Support relative addressing in preshader.

[Matteo Bruni]

2017-03-23 16:06 GMT+01:00 Paul Gofman :
> Signed-off-by: Paul Gofman
>
> dlls/d3dx9_36/preshader.c    | 103 ++++++++++++++++++++++++++++++++++++-------
>  dlls/d3dx9_36/tests/effect.c |  38 ++++++++--------
>  2 files changed, 106 insertions(+), 35 deletions(-)
>
> diff --git a/dlls/d3dx9_36/preshader.c b/dlls/d3dx9_36/preshader.c
> index 9aedc99..2974ffd 100644
> --- a/dlls/d3dx9_36/preshader.c
> +++ b/dlls/d3dx9_36/preshader.c
> @@ -191,6 +191,8 @@ struct d3dx_pres_operand
>      /* offset is component index, not register index, e. g.
>         offset for component c3.y is 13 (3 * 4 + 1) */
>      unsigned int offset;
> - enum pres_reg_tables base_reg_table;
> - unsigned int base_reg_offset;
>
> };

It would be nicer to create a struct for storing together table and
offset and to pass it around to the various functions taking both
arguments, like parse_pres_reg().
It should probably be a separate patch before this one.
> -
>    ptr = parse_pres_reg(ptr + 1, &opr->base_reg_table, &opr->base_reg_offset);
> - }
> - else
> - {
> -
>    opr->base_reg_table = PRES_REGTAB_COUNT;
> -
>    ++ptr;
> }
> - opr->table = reg_table[*ptr++];
> - opr->offset = *ptr++;
> -
> - ptr = parse_pres_reg(ptr, &opr->table, &opr->offset);

This would crash if the previous parse_pres_reg() call returned NULL.
> - size = rs->table_sizes[table] * table_info[table].reg_component_count;
> - if (offset >= size)
> - {
> -
>    if ((offset & 255) < size)
> -
>        offset &= 255;
> -
>    else
> -
>        offset = offset % size;
> - }

Is this just to try and avoid the modulo operation in some cases or is
there a deeper reason?
If the former, you could extend that to all the power-of-two sizes by
first checking if that's the case (!(size & size - 1)) and, if so,
and-ing offset with the mask (size - 1). That's assuming the compiler
doesn't do the same optimization already.

Let me know if there is some other reason instead.
> static double exec_get_arg(struct d3dx_regstore *rs, const struct d3dx_pres_operand *opr, unsigned int comp)
>  {
> - if (!regstore_is_val_set_reg(rs, opr->table, (opr->offset + comp) / table_info[opr->table].reg_component_count))
> -
>    WARN("Using uninitialized input, table %u, offset %u.\n", opr->table, opr->offset + comp);
> - unsigned int base_offset, index;
> -
> - if (opr->base_reg_table == PRES_REGTAB_COUNT)
> -
>    index = 0;
> - else
> -
>    index = (int)exec_get_reg_value(rs, opr->base_reg_table, opr->base_reg_offset);

I think it's better with lrint() in place of the cast.
> - if (index >= rs->table_sizes[opr->table] && opr->table == PRES_REGTAB_CONST)
> - {
> -
>    unsigned int size_pow2;
> -
> -
>    for (size_pow2 = 1; size_pow2 < rs->table_sizes[opr->table]; size_pow2 <<= 1)
> -
>        ;
> -
>    index &= (size_pow2 - 1);
> -
>    if (index >= rs->table_sizes[opr->table])
> -
>        return 0.0;
> - }

I don't think we necessarily have to follow native's madness on buffer
overflows to the letter. Since you did, that's certainly fine though.