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

[Paul Gofman]

On 08/29/2017 01:51 AM, Matteo Bruni wrote:
> 2017-08-28 23:32 GMT+02:00 Paul Gofman :
> > On 08/28/2017 09:00 PM, Matteo Bruni wrote:
> > > > @@ -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 ==
> > > - 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.
> >
> > This 'if' selects just preshader output constants. The entries for const_set
> > handled (merged) in this 'if' are those provided preshader instructions
> > which output anything to shader constants. They can potentially overlap
> > (that's why '>=' in the last 'if' check).  I am not sure what can be wrong
> > with that? We track preshader output as a single "commit", the same does
> > native implementation according to our tests.
>
> I don't have any issues with the if condition. It's the
> const_set[i].register_count assignment which looks troublesome since
> it doesn't consider the old count. In the (artificial, even forced if
> you want) example I gave const_set[i].register_count would go from 4
> to 2 which doesn't seem right.

Here is how this register_count is currently evaluated:

---- snip ----

const_set.register_count = max(get_reg_offset(reg->table,
             pres_op_info[ins->op].func_all_comps ? 1 : 
ins->component_count), 1);

---- snip ----

This expression is written for some generic case of component_count 
and components per register, but unless I am missing something this 
expression should not evaluate to anything but 1 for the real cases. We 
do not check that component count in instruction does not exceed 
component count per register during preshader parse though (just 
checking if it is between 1 and 4).

So maybe I will add an assert for the overlapping registers in 
this 'if' under discussion just in case?