2017-06-06 14:28 GMT+02:00 Paul Gofman gofmanp@gmail.com:
+static void pres_float_to_int(void *in, void *out, unsigned int count)
"in" should probably be const in all of these helpers.
+static void pres_bool_to_int(void *in, void * out, unsigned int count)
Stray whitespace after *.
+{
- 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)?
+static void regstore_set_data(struct d3dx_regstore *rs, unsigned int table,
unsigned int offset, unsigned int *in, unsigned int count, enum pres_value_type param_type)+{
- typedef void (*conv_func)(void *in, void *out, unsigned int count);
- static conv_func set_const_funcs[PRES_VT_COUNT][PRES_VT_COUNT] =
Missing const.
- {
{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).
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.
@@ -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.
} } 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.
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'?
2017-06-09 20:55 GMT+02:00 Paul Gofman gofmanp@gmail.com:
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.
Sounds fair.
- {
{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.
That would break in patch 2/5 (if it isn't updated accordingly) though, as I mentioned.
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.
No big difference in practice but it's quite a bit more awkward in that you're temporarily casting e.g. the float table to unsigned int, then finally converting data to float, in place.
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?
The assertion I envision would check that both the source and the destination data type have size == sizeof(unsigned int), which is what you're depending on. It could be a static assert (see e.g. C_ASSERT()), in principle.
@@ -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. */?
In that point of the code the "interesting" bit to note is that you're storing the "reshaped", but not converted, data temporarily in the final storage. Which isn't nice either way, but at least a comment reduces the surprise factor.
} } 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'?
Yeah, that's the "assert" I want. Not sure what's the best form for that, it doesn't necessarily have to be an assert().
On 06/09/2017 10:14 PM, Matteo Bruni wrote:
2017-06-09 20:55 GMT+02:00 Paul Gofman gofmanp@gmail.com:
That would break in patch 2/5 (if it isn't updated accordingly) though, as I mentioned.
Yes, sure, I will update the code accordingly in this patchset to do memcpy when it is required only.
The assertion I envision would check that both the source and the destination data type have size == sizeof(unsigned int), which is what you're depending on. It could be a static assert (see e.g. C_ASSERT()), in principle.
So I will put something like C_ASSERT(sizeof(BOOL) == sizeof(unsigned int) && sizeof(float) == sizeof(unsigned int) ...) in set_constants(), is it ok?
} } 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'?
Yeah, that's the "assert" I want. Not sure what's the best form for that, it doesn't necessarily have to be an assert().
I will update the patchset probably in a few days, as I am away now.
2017-06-09 21:38 GMT+02:00 Paul Gofman gofmanp@gmail.com:
On 06/09/2017 10:14 PM, Matteo Bruni wrote:
The assertion I envision would check that both the source and the destination data type have size == sizeof(unsigned int), which is what you're depending on. It could be a static assert (see e.g. C_ASSERT()), in principle.
So I will put something like C_ASSERT(sizeof(BOOL) == sizeof(unsigned int) && sizeof(float) == sizeof(unsigned int) ...) in set_constants(), is it ok?
That one is trivially correct and won't help in case the code changes down the line.
Something that checks that the size of a param->type component is the same as the size of a rs->tables[table] component is the same as the size of an unsigned int. That is what you depend on and that's what you should verify.
-
Matteo Bruni -
Paul Gofman