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

Matteo Bruni matteo.mystral at gmail.com
Tue Aug 29 09:35:34 CDT 2017


2017-08-29 1:16 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
> 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).

Right, I think checking that during parsing is a good idea regardless
of everything else.

I'd argue that either the expression above is too generic (allowing /
handling register_count > 1) or the other register_count expression
(the merge one I brought up earlier) is too naive, not expecting the
same. As it is it just doesn't look right.
If you ensure during parsing that, for each preshader instruction, 0 <
component_count <= components_per_register then you can just always
set the initial register_count to 1. Actually you could avoid setting
it at all at that point (more on that below).

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

That may also be a good idea. Otherwise, if the "+ out->const_set[i +
1].register_count" part is replaced with a "+ 1", the overlap case
would be handled trivially. That would make it possible to avoid the
previous register_count initialization entirely, assuming
register_count is set to 1 during merging in the NOT merging case.

Both options are okay with me. Also, as usual, let me know if I'm
still missing something.



More information about the wine-devel mailing list