[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 <gofmanp at gmail.com>:
>> 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 ==
>>> 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.
>> 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?