[PATCH 2/2] d3dx9: Get rid of constants modification bitmasks.

[Matteo Bruni]

2017-08-10 12:02 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
> Signed-off-by: Paul Gofman <gofmanp at gmail.com>

The patch is fine overall, I only have a few questions / nitpicks.
I'll skip the part about the delay with the review because it's old
news by now... :/

> ---
>  dlls/d3dx9_36/d3dx9_private.h |   3 +-
>  dlls/d3dx9_36/preshader.c     | 312 ++++++++++++++++++------------------------
>  2 files changed, 135 insertions(+), 180 deletions(-)
>
> diff --git a/dlls/d3dx9_36/d3dx9_private.h b/dlls/d3dx9_36/d3dx9_private.h
> index 2727463a18..5ac5e63aeb 100644
> --- a/dlls/d3dx9_36/d3dx9_private.h
> +++ b/dlls/d3dx9_36/d3dx9_private.h
> @@ -230,7 +230,7 @@ enum pres_reg_tables
>  struct d3dx_const_param_eval_output
>  {
>      struct d3dx_parameter *param;
> -    unsigned int table;
> +    enum pres_reg_tables table;

The change is good but (unrelated to this patch) the enum type should
really use the singular form.

>  static void dump_bytecode(void *data, unsigned int size)
> @@ -674,6 +614,40 @@ static HRESULT append_const_set(struct d3dx_const_tab *const_tab, struct d3dx_co
>      return D3D_OK;
>  }
>
> +static void append_pres_const_set(struct d3dx_const_tab *const_tab, struct d3dx_preshader *pres)

Nitpick: this function appends all the struct
d3dx_const_param_eval_output from the preshader so calling it
"append_pres_const_sets" (or even something more explicit to that
effect, e.g. clarifying that you're appending entries about the
preshader outputs, which are shader inputs) would be preferable.

> +{
> +    unsigned int i;
> +    struct d3dx_const_param_eval_output const_set;
> +
> +    memset(&const_set, 0, sizeof(const_set));

You could initialize const_set in the definition instead, like:

struct d3dx_const_param_eval_output const_set = {NULL};

which looks slightly better to me. It doesn't matter much of course.

> @@ -971,10 +946,30 @@ static HRESULT get_constants_desc(unsigned int *byte_code, struct d3dx_const_tab
>          if (FAILED(hr = init_set_constants_param(out, ctab, hc, inputs_param[index])))
>              goto cleanup;
>      }
> +    if (pres)
> +        append_pres_const_set(out, pres);
>      if (out->const_set_count)
>      {
>          struct d3dx_const_param_eval_output *new_alloc;
>
> +        qsort(out->const_set, out->const_set_count, sizeof(*out->const_set), compare_const_set);
> +        for (i = 0; i < out->const_set_count - 1; ++i)
> +        {
> +            if (out->const_set[i].constant_class == D3DXPC_FORCE_DWORD
> +                    && out->const_set[i + 1].constant_class == D3DXPC_FORCE_DWORD
> +                    && out->const_set[i].table == out->const_set[i + 1].table
> +                    && out->const_set[i].register_index + out->const_set[i].register_count
> +                    >= out->const_set[i + 1].register_index)
> +            {
> +                out->const_set[i].register_count = out->const_set[i + 1].register_index
> +                        + out->const_set[i + 1].register_count - out->const_set[i].register_index;

I think this might lose account of some registers. E.g. consider the
case where const_set[i] has register_index == 0 and register_count ==
4 while const_set[i + 1] has register_index == 1 and register_count ==
1. This would seem to lose track of the last two components.

I know that preshaders found in the wild seem not to overwrite already
written output registers but I wouldn't feel safe with (silently)
depending on it.

> +                memmove(&out->const_set[i + 1], &out->const_set[i + 2], sizeof(out->const_set[i])
> +                        * (out->const_set_count - i - 2));
> +                --out->const_set_count;
> +                --i;

Mostly matter of personal preference but rather than decrementing the
for counter in the if when coalescing I'd write this as a while and
increment "i" only in the (currently absent) else case.

> @@ -1529,22 +1534,56 @@ static void set_constants(struct d3dx_regstore *rs, struct d3dx_const_tab *const
>              data += param->rows * param->columns;
>          }
>          start_offset = get_offset_reg(table, const_set->register_index);
> -        if (table_info[table].type == param_type)
> -            regstore_set_modified(rs, table, start_offset,
> -                    get_offset_reg(table, const_set->register_count) * const_set->element_count);
> -        else
> +        if (table_info[table].type != param_type)
>              regstore_set_data(rs, table, start_offset, (unsigned int *)rs->tables[table] + start_offset,
>                      get_offset_reg(table, const_set->register_count) * const_set->element_count, param_type);
>      }
> +
> +
>      const_tab->update_version = new_update_version;

One blank line too many.

> -}
> +    if (!update_device)
> +        return D3D_OK;
>
> +    for (const_idx = 0; const_idx < const_tab->const_set_count; ++const_idx)
> +    {
> +        struct d3dx_const_param_eval_output *const_set = &const_tab->const_set[const_idx];
> +
> +        if (device_update_all || (const_set->param
> +                ? is_param_dirty(const_set->param, update_version) : pres_dirty))
> +        {
> +            enum pres_reg_tables table = const_set->table;
> +
> +            if (table == device_table && device_start + device_count == const_set->register_index)

Again nitpicking: are there better names for those variables? Maybe
use the "current_" prefix instead? It's pretty clear what's going on
either way, so up to you.