Re: [v9 3/6] d3dx9: Implement fxlc constants (expressions) in effect.
2016-04-01 13:21 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
+static double regstore_get_double(struct d3dx_regstore *rs, unsigned int table, unsigned int offset) +{ + BYTE *p; + + p = (BYTE *)rs->tables[table] + table_info[table].component_size * offset; + switch (table_info[table].type) + { + case PRES_VT_FLOAT : return *(float *)p; + case PRES_VT_DOUBLE: return *(double *)p; + case PRES_VT_INT : return *(int *)p; + case PRES_VT_BOOL : return *(BOOL *)p ? 1.0f : 0.0f;
The 'f' suffix is now wrong since you're returning double. Something like "return !!*(BOOL *)p;" would be probably preferable, although I don't think you're ever supposed to read from PRES_VT_INT or PRES_VT_BOOL tables so maybe replacing the last two cases with a FIXME or similar is even better.
+static void regstore_set_double(struct d3dx_regstore *rs, unsigned int table, unsigned int offset, double v) +{ + BYTE *p; + unsigned int reg_idx; + + p = (BYTE *)rs->tables[table] + table_info[table].component_size * offset; + switch (table_info[table].type) + { + case PRES_VT_FLOAT : *(float *)p = v; break; + case PRES_VT_DOUBLE: *(double *)p = v; break; + case PRES_VT_INT : *(int *)p = roundf(v); break;
Same here. Also it's not clear that rounding away from 0 is correct, this would need tests (although IIRC preshaders generated by the MS compiler explicitly compute integral values for integer constants, that would make this hard to test and mostly irrelevant in practice). So please try to test for rounding behavior with 0.5 / -0.5 / 1.5, if nothing else to confirm that my memory serves me right. BTW unless testing proves otherwise lrint() is probably the best option here since it generates much nicer code (at least when building with gcc for 32 bits).
+ case PRES_VT_BOOL : *(BOOL *)p = !!v; break;
The !! here might also be unnecessary strictly speaking but this isn't quite as significant and it's okay to me either way.
+ transpose = (desc.Class == D3DXPC_MATRIX_COLUMNS && param->class == D3DXPC_MATRIX_ROWS) || + (param->class == D3DXPC_MATRIX_COLUMNS && desc.Class == D3DXPC_MATRIX_ROWS);
It looks like you forgot to move the operator at the start of the next line here (while you did that e.g. in patch 6/6).
On 04/06/2016 04:42 PM, Matteo Bruni wrote:
+static void regstore_set_double(struct d3dx_regstore *rs, unsigned int table, unsigned int offset, double v) +{ + BYTE *p; + unsigned int reg_idx; + + p = (BYTE *)rs->tables[table] + table_info[table].component_size * offset; + switch (table_info[table].type) + { + case PRES_VT_FLOAT : *(float *)p = v; break; + case PRES_VT_DOUBLE: *(double *)p = v; break; + case PRES_VT_INT : *(int *)p = roundf(v); break; Same here. Also it's not clear that rounding away from 0 is correct, this would need tests (although IIRC preshaders generated by the MS compiler explicitly compute integral values for integer constants, that would make this hard to test and mostly irrelevant in practice). So please try to test for rounding behavior with 0.5 / -0.5 / 1.5, if nothing else to confirm that my memory serves me right. BTW unless testing proves otherwise lrint() is probably the best option here since it generates much nicer code (at least when building with gcc for 32 bits).
In fact the main test now does not have integer constants. Preshader code I've seen generated by MS compiler so far indeed does not use any rounding functions, doing some math with fract, neg and min/max instead. I already have some further patches involving tests of the newly added operations which do not overwrite the blob but modify the opcode in place. I used the same mini framework to test the other edge case which is currently not in the main test: 0 * INF (BTW the results are funny, preshaders exhibit IEEE FP behaviour, like in my implementation, while d3d9 shaders have 0 * INF == 0). I suppose it will be easier to send and less code changes to review if we consider the whole updated test blob with all the planned changes at once afterwards, can we go this way?
2016-04-07 10:37 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 04/06/2016 04:42 PM, Matteo Bruni wrote:
+static void regstore_set_double(struct d3dx_regstore *rs, unsigned int table, unsigned int offset, double v) +{ + BYTE *p; + unsigned int reg_idx; + + p = (BYTE *)rs->tables[table] + table_info[table].component_size * offset; + switch (table_info[table].type) + { + case PRES_VT_FLOAT : *(float *)p = v; break; + case PRES_VT_DOUBLE: *(double *)p = v; break; + case PRES_VT_INT : *(int *)p = roundf(v); break; Same here. Also it's not clear that rounding away from 0 is correct, this would need tests (although IIRC preshaders generated by the MS compiler explicitly compute integral values for integer constants, that would make this hard to test and mostly irrelevant in practice). So please try to test for rounding behavior with 0.5 / -0.5 / 1.5, if nothing else to confirm that my memory serves me right. BTW unless testing proves otherwise lrint() is probably the best option here since it generates much nicer code (at least when building with gcc for 32 bits).
In fact the main test now does not have integer constants. Preshader code I've seen generated by MS compiler so far indeed does not use any rounding functions, doing some math with fract, neg and min/max instead. I already have some further patches involving tests of the newly added operations which do not overwrite the blob but modify the opcode in place. I used the same mini framework to test the other edge case which is currently not in the main test: 0 * INF (BTW the results are funny, preshaders exhibit IEEE FP behaviour, like in my implementation, while d3d9 shaders have 0 * INF == 0). I suppose it will be easier to send and less code changes to review if we consider the whole updated test blob with all the planned changes at once afterwards, can we go this way?
That sounds about what I expected and using lrint() here seems ideal. Yes, the additional tests can come after this set of patches.
participants (2)
-
Matteo Bruni -
Paul Gofman