Re: [v10 1/4] d3dx9: Implement fxlc constants (expressions) in effect.
2016-04-07 17:41 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
switch (state->type) { case ST_CONSTANT: + *param_value = param->data; + *out_param = param; + return D3D_OK; case ST_PARAMETER: + param = param->referenced_param; *param_value = param->data; *out_param = param; return D3D_OK;
It doesn't matter (this is perfectly fine) but you could have kept the fallthrough by switching these two cases around.
+static HRESULT set_constants_param(struct d3dx_regstore *rs, struct d3dx_const_tab *const_tab, + D3DXHANDLE hc, struct d3dx_parameter *param) +{ + ID3DXConstantTable *ctab = const_tab->ctab; + D3DXCONSTANT_DESC desc; + unsigned int const_count, param_count, i, j, n, table, start_offset; + unsigned int minor, major, major_stride, param_offset; + BOOL transpose, get_element; + + ID3DXConstantTable_GetConstantDesc(ctab, hc, &desc, &const_count);
This isn't correct, although it currently works. const_count is supposed to be an inout parameter for GetConstantDesc() (you could potentially have multiple instances of the same constant in different register sets). The current implementation of GetConstantDesc() doesn't check the parameter and blindly returns only the first descriptor, also setting const_count to 1 (actually the following descriptors aren't even parsed currently by the constant table code IIRC) so this ends up working, although that's a bug and at some point it might get fixed which would in turn break this. Also it's a bit of an "overload" to use the same variable here and as the count of constant elements / members in the following lines, it's probably nicer to use two separate variables. BTW you're using GetConstantDesc() correctly in get_constants_desc().
+ table = const_tab->regset2table[desc.RegisterSet]; + if (table >= PRES_REGTAB_COUNT) + { + FIXME("Unexpected register set %u.\n", desc.RegisterSet); + return D3DERR_INVALIDCALL; + }
Since regset2table is defined by us ERR() is maybe more appropriate here. It might make sense to also check desc.RegisterSet before accessing the array.
+ hc = ID3DXConstantTable_GetConstant(const_tab->ctab, NULL, i); + if (!hc) + { + FIXME("Could not get constant.\n"); + hr = D3DERR_INVALIDCALL;
You could probably also print the value of 'i' in the FIXME. Up to you, but you could switch the then and else branches and drop the '!' in the if condition.
+ if (oi->func_all_comps) + { + if (oi->input_count * ins->component_count > ARGS_ARRAY_SIZE) + { + FIXME("Too many arguments (%u) for one instruction.\n", oi->input_count * ins->component_count); + return E_FAIL; + }
I think this check should be outside of the if (oi->func_all_comps) block.
+ hr = set_constants(&peval->pres.regs, &peval->pres.inputs); + if (FAILED(hr)) + return hr; + + hr = execute_preshader(&peval->pres); + if (FAILED(hr)) + return hr;
Not a big deal but since you're there... You could merge the function calls with the respective if, like you're doing in some other patches of the series.
On 04/11/2016 07:52 PM, Matteo Bruni wrote:
2016-04-07 17:41 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
+static HRESULT set_constants_param(struct d3dx_regstore *rs, struct d3dx_const_tab *const_tab, + D3DXHANDLE hc, struct d3dx_parameter *param) +{ + ID3DXConstantTable *ctab = const_tab->ctab; + D3DXCONSTANT_DESC desc; + unsigned int const_count, param_count, i, j, n, table, start_offset; + unsigned int minor, major, major_stride, param_offset; + BOOL transpose, get_element; + + ID3DXConstantTable_GetConstantDesc(ctab, hc, &desc, &const_count); This isn't correct, although it currently works. const_count is supposed to be an inout parameter for GetConstantDesc() (you could potentially have multiple instances of the same constant in different register sets). The current implementation of GetConstantDesc() doesn't check the parameter and blindly returns only the first descriptor, also setting const_count to 1 (actually the following descriptors aren't even parsed currently by the constant table code IIRC) so this ends up working, although that's a bug and at some point it might get fixed which would in turn break this. Also it's a bit of an "overload" to use the same variable here and as the count of constant elements / members in the following lines, it's probably nicer to use two separate variables.
BTW you're using GetConstantDesc() correctly in get_constants_desc().
I was looking into GetConstantsDesc() implementation to get the idea how constants count can be > 1 there, and saw there that the count is ultimately 1, that's why left it like that. Do you think it is OK just to set count to 1 prior to the call? As I get MSDN text for this count > 1 is possible only for samplers and we should not get here for samplers anyway.
On 04/11/2016 08:05 PM, Paul Gofman wrote:
On 04/11/2016 07:52 PM, Matteo Bruni wrote:
2016-04-07 17:41 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
+static HRESULT set_constants_param(struct d3dx_regstore *rs, struct d3dx_const_tab *const_tab, + D3DXHANDLE hc, struct d3dx_parameter *param) +{ + ID3DXConstantTable *ctab = const_tab->ctab; + D3DXCONSTANT_DESC desc; + unsigned int const_count, param_count, i, j, n, table, start_offset; + unsigned int minor, major, major_stride, param_offset; + BOOL transpose, get_element; + + ID3DXConstantTable_GetConstantDesc(ctab, hc, &desc, &const_count); This isn't correct, although it currently works. const_count is supposed to be an inout parameter for GetConstantDesc() (you could potentially have multiple instances of the same constant in different register sets). The current implementation of GetConstantDesc() doesn't check the parameter and blindly returns only the first descriptor, also setting const_count to 1 (actually the following descriptors aren't even parsed currently by the constant table code IIRC) so this ends up working, although that's a bug and at some point it might get fixed which would in turn break this. Also it's a bit of an "overload" to use the same variable here and as the count of constant elements / members in the following lines, it's probably nicer to use two separate variables.
BTW you're using GetConstantDesc() correctly in get_constants_desc().
I was looking into GetConstantsDesc() implementation to get the idea how constants count can be > 1 there, and saw there that the count is ultimately 1, that's why left it like that. Do you think it is OK just to set count to 1 prior to the call? As I get MSDN text for this count > 1 is possible only for samplers and we should not get here for samplers anyway.
Oh, please ignore this, actually the value type constant may also appear multiple types if it is used in different register index. So I suggest to initialize count to 1 before the call and put a FIXME if count != 1 on return. Making the real implementation for > 1 will be just a dead code for now until GetConstantsDesc actually support this.
On 04/11/2016 07:52 PM, Matteo Bruni wrote:
+ if (oi->func_all_comps) + { + if (oi->input_count * ins->component_count > ARGS_ARRAY_SIZE) + { + FIXME("Too many arguments (%u) for one instruction.\n", oi->input_count * ins->component_count); + return E_FAIL; + } I think this check should be outside of the if (oi->func_all_comps) block.
For 'else' case all the components are not put in this array together, only oi->input_count arguments at once. I could add such a check in else but this would effectively be a plain static check that ARGS_ARRAY_SIZE >= MAX_INPUTS_COUNT, so I left this unchanged for now. BTW I encountered d3ts_dotswiz preshader opcode which can take 6 or 8 arguments. Adding it required the appropriate increase of MAX_INPUTS_COUNT and a bit different handling of op table (not to introduce a variable input count in instructions and not to take back the input arguments count into instruction structure), but ARGS_ARRAY_SIZE remained unchanged.
2016-04-12 11:41 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 04/11/2016 07:52 PM, Matteo Bruni wrote:
+ if (oi->func_all_comps) + { + if (oi->input_count * ins->component_count > ARGS_ARRAY_SIZE) + { + FIXME("Too many arguments (%u) for one instruction.\n", oi->input_count * ins->component_count); + return E_FAIL; + } I think this check should be outside of the if (oi->func_all_comps) block.
For 'else' case all the components are not put in this array together, only oi->input_count arguments at once. I could add such a check in else but this would effectively be a plain static check that ARGS_ARRAY_SIZE >= MAX_INPUTS_COUNT, so I left this unchanged for now.
Right, I probably misread that.
BTW I encountered d3ts_dotswiz preshader opcode which can take 6 or 8 arguments. Adding it required the appropriate increase of MAX_INPUTS_COUNT and a bit different handling of op table (not to introduce a variable input count in instructions and not to take back the input arguments count into instruction structure), but ARGS_ARRAY_SIZE remained unchanged.
Cool. How did you test that 6 and 8 are the only possible argument counts? I don't know how you changed the opcode handling but otherwise you could probably just use two separate opcodes for dotswiz 6 and dotswiz 8.
On 04/13/2016 10:53 PM, Matteo Bruni wrote:
BTW I encountered d3ts_dotswiz preshader opcode which can take 6 or 8 arguments. Adding it required the appropriate increase of MAX_INPUTS_COUNT and a bit different handling of op table (not to introduce a variable input count in instructions and not to take back the input arguments count into instruction structure), but ARGS_ARRAY_SIZE remained unchanged.
Cool. How did you test that 6 and 8 are the only possible argument counts? I don't know how you changed the opcode handling but otherwise you could probably just use two separate opcodes for dotswiz 6 and dotswiz 8.
I checked the translation all possible components count constructs like a.zxyy * b.xxxw. If the components count is 3 or 4, MS compiler generates d3ts_dotswiz with the respective number of args, and plain mul/add if less. It does not prove that it is not possible to get d3ts_dotswiz with 2 though, but I am not sure I should add such until I will be sweeping all possible opcodes to have all of them in place. After we finish with this patchset I will summarize the leftovers/TODOs for preshader/effects I know so we could possibly prioritize them. I used two separate entries into op_info with the same opcode and different args count, and modified search to compare both opcode and number of arguments.
2016-04-13 22:16 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
On 04/13/2016 10:53 PM, Matteo Bruni wrote:
BTW I encountered d3ts_dotswiz preshader opcode which can take 6 or 8 arguments. Adding it required the appropriate increase of MAX_INPUTS_COUNT and a bit different handling of op table (not to introduce a variable input count in instructions and not to take back the input arguments count into instruction structure), but ARGS_ARRAY_SIZE remained unchanged.
Cool. How did you test that 6 and 8 are the only possible argument counts? I don't know how you changed the opcode handling but otherwise you could probably just use two separate opcodes for dotswiz 6 and dotswiz 8.
I checked the translation all possible components count constructs like a.zxyy * b.xxxw. If the components count is 3 or 4, MS compiler generates d3ts_dotswiz with the respective number of args, and plain mul/add if less. It does not prove that it is not possible to get d3ts_dotswiz with 2 though, but I am not sure I should add such until I will be sweeping all possible opcodes to have all of them in place. After we finish with this patchset I will summarize the leftovers/TODOs for preshader/effects I know so we could possibly prioritize them.
I used two separate entries into op_info with the same opcode and different args count, and modified search to compare both opcode and number of arguments.
Sounds good, thanks!
participants (2)
-
Matteo Bruni -
Paul Gofman