[PATCH 1/5] d3dx9: Avoid casting each value separately in set_constants().

[Paul Gofman]

On 06/09/2017 08:49 PM, Matteo Bruni wrote:
>
>> +{
>> +    unsigned int i;
>> +    const float *in_bool = in;
>> +    int *out_int = out;
>> +
>> +    for (i = 0; i < count; ++i)
>> +        out_int[i] = !!in_bool[i];
> Is the double negation necessary here (or above)?
In the current implementation such a conversion ended up in get_bool() 
from utils.c, which did "*(DWORD *)data != 0" for BOOLs also. I did not 
want to introduce side changes in this purely optimization patch.
>
>
>> +    {
>> +        {NULL,               NULL, pres_float_to_int, pres_value_to_bool},
>> +        {NULL,               NULL, NULL,              NULL},
>> +        {pres_int_to_float,  NULL, NULL,              pres_value_to_bool},
>> +        {pres_bool_to_float, NULL, pres_bool_to_int,  NULL}
>> +    };
>> +    enum pres_value_type table_type = table_info[table].type;
>> +
>> +    if (table_type == param_type)
>> +    {
>> +        regstore_set_values(rs, table, in, offset, count);
>> +        return;
>> +    }
> IIUC in this case regstore_set_values() will have in == out. That's a
> problem since it means memcpy()ing the data onto itself, which is
> undefined. It's also unnecessary, regstore_set_modified() would be
> enough, but only if in == out which is something the current
> regstore_set_data() interface doesn't enforce (and indeed patch 2/5
> doesn't respect).
Oh yes, I actually want regstore_set_modified() here. In the next 
patches (which I did not send yet) where I get rid of modified bitmasks 
entirely this just goes away completely.
>
> I guess the root issue is that you're now using the regstore also as
> temporary storage and none of the existing (and new) code expects it.
> So it's a matter of fixing the code as necessary to make it work fine
> or to avoid this "new" usage of the regstore e.g. by means of a
> separate temporary buffer.
>
>>           for (element = 0; element < const_set->element_count; ++element)
>>           {
>> +            unsigned int *out = (unsigned int *)rs->tables[table] + start_offset;
> Aside from the issue I mentioned above, this works only because all
> the formats we're interesting in converting from / to happen be 4
> bytes wide. It's somewhat assumed in a bunch of places already but
> adding more isn't too exciting. I'd like at least an assert checking
> for the precondition, if possible.
It was assumed exactly the same way before this patch, there was 
'unsigned int out' variable which was holding conversion result for any 
type, so this patch does not change nothing. I can add an assertion of 
course, but this is a sort of static assert. Can I do it as #if ... 
#error, or place it in initialization? Don't you think it will look a 
bit ugly to check the size of the list of predefined types in the code? 
Or am I misunderstanding entirely how do you mean to assert this?
>
>> @@ -1083,23 +1155,15 @@ static void set_constants(struct d3dx_regstore *rs, struct d3dx_const_tab *const
>>                           WARN("Parameter data is too short, name %s, component %u.\n", debugstr_a(param->name), i);
>>                           break;
>>                       }
>> -
>> -                    in = (unsigned int *)data + param_offset;
>> -                    switch (table_type)
>> -                    {
>> -                        case PRES_VT_FLOAT: set_number(&out, D3DXPT_FLOAT, in, param->type); break;
>> -                        case PRES_VT_INT: set_number(&out, D3DXPT_INT, in, param->type); break;
>> -                        case PRES_VT_BOOL: set_number(&out, D3DXPT_BOOL, in, param->type); break;
>> -                        default:
>> -                            FIXME("Unexpected type %#x.\n", table_info[table].type);
>> -                            break;
>> -                    }
>> -                    regstore_set_values(rs, table, &out, offset, 1);
>> +                    out[offset] = data[param_offset];
> I think separating the matrix transposing / "reshaping" from the data
> conversion is okay, if that allows further changes and optimizations
> like patch 2/5. You might want to add a small comment around here to
> clarify the split.
Well, it is already a huge optimization even without further patches, as 
it removes switch and function call for each value being set, and does 
this just once for a set of values. Should I add some comment like /* Do 
the conversion once for the array of values. */?
>
>>                   }
>>               }
>>               start_offset += get_offset_reg(table, const_set->register_count);
>>               data += param->rows * param->columns;
>>           }
>> +        start_offset = get_offset_reg(table, const_set->register_index);
>> +        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);
> I understand that you want to avoid to use some separate storage for
> the "to be converted" data but, like above, this regstore table
> pointer cast makes me a bit uncomfortable.
>
Or yeah, I really want to avoid extra storage for conversion, it adds 
extra memory access and affects performance a bit (I had such variant as 
a first time implementation). Same as above, it is essentially the same 
as it was before, just previously it was a single "unsigned int out;" 
while now it is an array. Maybe then I will add runtime check (or 
preprocessor check) in initialization that all the supported types 
really have the same size as 'unsigned int'?