2017-08-29 1:16 GMT+02:00 Paul Gofman gofmanp@gmail.com:
On 08/29/2017 01:51 AM, Matteo Bruni wrote:
2017-08-28 23:32 GMT+02:00 Paul Gofman gofmanp@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 ==
- 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 andcomponents 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.