Re: [v3 resend 3/6] d3dx9: implement fxlc constants (expressions) in effect.

[Matteo Bruni]

2016-03-10 19:50 GMT+01:00 Paul Gofman :
> Signed-off-by: Paul Gofman
> +static float regstore_get_float(struct d3dx_regstore *rs, unsigned int table, unsigned int offset)
> +{
> - BYTE *p;
> -
> - p = (BYTE *)rs->tables[table] + table_info[table].value_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;

I guess that -0.0f comes from some test?
> -
>    /* no EOL in WARN to dump arg in the same line */
> -
>    WARN("Using uninitialized input ");
> -
>    dump_arg(rs, opr, comp);
> -
>    TRACE(".\n");
> -
>    dump_ins(rs, ins);

You can probably do without the comment, it's pretty clear anyway.
> +#define ARGS_ARRAY_SIZE 8
> +static HRESULT execute_preshader(struct d3dx_preshader *pres)
> +{
> - unsigned int i, j, k;
> - float args[ARGS_ARRAY_SIZE];
> - float res;
> -
> - for (i = 0; i < pres->nins; i++)
> - {
> -
>    struct d3dx_pres_ins *ins;
> -
>    struct op_info *oi;
> -
> -
>    ins = &pres->ins[i];
> -
>    oi = &pres_op_info[ins->op];
> -
>    if (oi->func_all_comps)
> -
>    {
> -
>        if (ins->ninp_args * ins->ncomps > ARGS_ARRAY_SIZE)
> -
>        {
> -
>            FIXME("Too much arguments (%u) for one instruction.\n", ins->ninp_args * ins->ncomps);

That should be "Too many".
> +HRESULT d3dx_param_eval_set_shader_constants(struct IDirect3DDevice9 *device, struct d3dx_param_eval *peval)

This function is unused in this patch. Please move it to the first
patch requiring it (I think that's patch 6/6). BTW, splitting patch
6/6 would be nice although I don't have any suggestions on how to do
that in a sensible manner...
> +HRESULT d3dx_param_eval_get_shader_parameters(struct d3dx_param_eval *peval, struct d3dx_parameter ***param,
> -
>    unsigned int *nparam, D3DXCONSTANT_DESC **cdesc)
>
> +{
> - *nparam = peval->shader_inputs.ninputs;
> - *param = peval->shader_inputs.inputs_param;
> - *cdesc = peval->shader_inputs.inputs;
> - return D3D_OK;
>
> +}

You don't use this function in this patch either. Also it doesn't seem
useful, it isn't an improvement to just assigning those three pointers
directly in the caller IMO.

[v3 resend 3/6] d3dx9: implement fxlc constants (expressions) in effect.

[Paul Gofman]

On 03/15/2016 05:54 AM, Matteo Bruni wrote:
> +static float regstore_get_float(struct d3dx_regstore *rs, unsigned int table, unsigned int offset)
> +{
> - BYTE *p;
> -
> - p = (BYTE *)rs->tables[table] + table_info[table].value_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;
>
> I guess that -0.0f comes from some test?

No, I just saw a lot of constructs in preshader disassembly like "cmp
r1.x, c0.x, 1.0, -0.0", somehow they like to put -0.0 for immediate
constants for such sort of cmp, and was thinking of this when I was
writing it. There is currently no usecase I know for preshader inputting
booleans or ints, so I can't test this. So I think this just need to be
changed to normal 0.0.
> +HRESULT d3dx_param_eval_get_shader_parameters(struct d3dx_param_eval *peval, struct d3dx_parameter ***param,
> -
>    unsigned int *nparam, D3DXCONSTANT_DESC **cdesc)
>
> +{
> - *nparam = peval->shader_inputs.ninputs;
> - *param = peval->shader_inputs.inputs_param;
> - *cdesc = peval->shader_inputs.inputs;
> - return D3D_OK;
>
> +}
> You don't use this function in this patch either. Also it doesn't seem
> useful, it isn't an improvement to just assigning those three pointers
> directly in the caller IMO.

I would like to remove this function but then I will need to put the
param_eval structure into shared header file (now there is only a
forward declaration). Is it OK?

[v3 resend 3/6] d3dx9: implement fxlc constants (expressions) in effect.

[Matteo Bruni]

2016-03-15 8:33 GMT+01:00 Paul Gofman :
> On 03/15/2016 05:54 AM, Matteo Bruni wrote:
> > +static float regstore_get_float(struct d3dx_regstore *rs, unsigned int table, unsigned int offset)
> > +{
> > - BYTE *p;
> > -
> > - p = (BYTE *)rs->tables[table] + table_info[table].value_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;
> >
> > I guess that -0.0f comes from some test?
>
> No, I just saw a lot of constructs in preshader disassembly like "cmp
> r1.x, c0.x, 1.0, -0.0", somehow they like to put -0.0 for immediate
> constants for such sort of cmp, and was thinking of this when I was
> writing it. There is currently no usecase I know for preshader inputting
> booleans or ints, so I can't test this. So I think this just need to be
> changed to normal 0.0.
> > +HRESULT d3dx_param_eval_get_shader_parameters(struct d3dx_param_eval *peval, struct d3dx_parameter ***param,
> > -
> >    unsigned int *nparam, D3DXCONSTANT_DESC **cdesc)
> >
> > +{
> > - *nparam = peval->shader_inputs.ninputs;
> > - *param = peval->shader_inputs.inputs_param;
> > - *cdesc = peval->shader_inputs.inputs;
> > - return D3D_OK;
> >
> > +}
> > You don't use this function in this patch either. Also it doesn't seem
> > useful, it isn't an improvement to just assigning those three pointers
> > directly in the caller IMO.
>
> I would like to remove this function but then I will need to put the
> param_eval structure into shared header file (now there is only a
> forward declaration). Is it OK?

Sure. Hopefully it doesn't bring too much stuff in from preshader.c
(but it's not a problem if it does, we don't need to hide preshader
internals from effect.c).