[PATCH 2/3] d3dx9: Support relative addressing in preshader.

[Matteo Bruni]

2017-03-24 2:16 GMT+01:00 Paul Gofman <gofmanp at gmail.com>:
> On 03/24/2017 03:31 AM, Matteo Bruni wrote:
>>
>> 2017-03-24 0:39 GMT+01:00 Paul Gofman <gofmanp at gmail.com>:
>>>
>>> On 03/24/2017 01:57 AM, Matteo Bruni wrote:
>>>>
>>>> 2017-03-23 16:06 GMT+01:00 Paul Gofman <gofmanp at gmail.com>:
>>>>>
>>>>> Signed-off-by: Paul Gofman <gofmanp at gmail.com>
>>>>> ---
>>>>>    dlls/d3dx9_36/preshader.c    | 103
>>>>> ++++++++++++++++++++++++++++++++++++-------
>>>>>    dlls/d3dx9_36/tests/effect.c |  38 ++++++++--------
>>>>>    2 files changed, 106 insertions(+), 35 deletions(-)
>>>>>
>>>>> diff --git a/dlls/d3dx9_36/preshader.c b/dlls/d3dx9_36/preshader.c
>>>>> index 9aedc99..2974ffd 100644
>>>>> --- a/dlls/d3dx9_36/preshader.c
>>>>> +++ b/dlls/d3dx9_36/preshader.c
>>>>> @@ -191,6 +191,8 @@ struct d3dx_pres_operand
>>>>>        /* offset is component index, not register index, e. g.
>>>>>           offset for component c3.y is 13 (3 * 4 + 1) */
>>>>>        unsigned int offset;
>>>>> +    enum pres_reg_tables base_reg_table;
>>>>> +    unsigned int base_reg_offset;
>>>>>    };
>>>>
>>>> It would be nicer to create a struct for storing together table and
>>>> offset and to pass it around to the various functions taking both
>>>> arguments, like parse_pres_reg().
>>>> It should probably be a separate patch before this one.
>>>
>>> Actually I can't guess the case when base_reg_table would be not a
>>> floating
>>> constant preshader input. Maybe I just remove this variable and check
>>> with
>>> FIXME during parsing?
>>
>> Sure, that's fine. Still the struct seems useful for parameter
>> passing, if not right there in struct d3dx_pres_operand.
>>
> Sorry, I messed up things actually, base register actually can come from
> temporary (register) table also, which happens in the test. So I will
> introduce extra structure for registers.

So I had a look at my stuff. As you know it has got very little and
specific testing and is most likely missing some cases but I see that:
- for relative addressing I accept any register type as base but
expect only temporary registers as index (i.e. I look for "flags" == 1
and then for the "base" fields only when parsing TEMP registers)
- for CONST registers the "flags" != 0 field has a different meaning.
In that case it is an offset in the IMMED table and there is no
additional "base" register type + index in the effect bytecode, so
it's indexed like imm["flags" + cidx].

Unfortunately I don't think I have any effect with the latter feature
around. Supporting that should probably be a separate patch but try to
see if you can replicate that.

>>>>>    static double exec_get_arg(struct d3dx_regstore *rs, const struct
>>>>> d3dx_pres_operand *opr, unsigned int comp)
>>>>>    {
>>>>> -    if (!regstore_is_val_set_reg(rs, opr->table, (opr->offset + comp)
>>>>> /
>>>>> table_info[opr->table].reg_component_count))
>>>>> -        WARN("Using uninitialized input, table %u, offset %u.\n",
>>>>> opr->table, opr->offset + comp);
>>>>> +    unsigned int base_offset, index;
>>>>> +
>>>>> +    if (opr->base_reg_table == PRES_REGTAB_COUNT)
>>>>> +        index = 0;
>>>>> +    else
>>>>> +        index = (int)exec_get_reg_value(rs, opr->base_reg_table,
>>>>> opr->base_reg_offset);
>>>>
>>>> I think it's better with lrint() in place of the cast.
>>>
>>> I just want to note that whenever the input is from floating values FX
>>> compiler generates preshader instructions which do the rounding, and
>>> skips
>>> that if the base register comes from integers. Do you think we need to do
>>> extra rounding in this case?
>>
>> Ah, you're right, I somehow overlooked that. Actually I think
>> doublechecking (e.g. with a FIXME) that the source is really integer
>> would be useful. If it isn't too much of a mess it would be nice to
>> avoid the roundtrip to doubles for the index entirely.
>
>     Source of the base register value seems not so straightforward to check.
> The input table always has float format. To do such check, we need to find
> which parameter the register is initialized from, we don't have such a link
> now.

Right, that's hard to do in the current architecture. Also I confused
this with the IMMED relative addressing I mentioned above, for those
my code expects the input CONST register to be mapped to an INT
parameter. That's only relevant in my architecture since I don't fill
all the input registers before executing the preshader but go fetch
them "on-the-fly" for each instruction. That is also clearly worse
than your solution, of course.
Long story short, just ignore this.

> Besides, using float register as index seems totally ok in effect,
> compiler generates rounding code for this case. Moreover, base register used
> in instruction may come from temporary ("r") registers and be a result of
> some operations, like rounding or multiplication when indexing anything with
> stride other than 4xfloat.

Right as well. I don't know, lrintf() seems like a good idea still.

>     Or could it be you had in mind not checking the source of register
> value, but rather that the value is actually an integer (fractional part is
> zero)?

That might be a bit fragile (because of potential numerical accuracy
troubles), although as you noticed the compiler seems to go to great
lengths to make sure the value is exactly an integer. Maybe doing that
kind of check only in a if (WARN_ON(d3dx)) might make sense as it
would be somewhat surprising and it might reveal something else going
on but it doesn't seem worth it to me in general.