[v2 1/2] d3dx9: Support relative addressing in preshader.

[Paul Gofman]

On 03/29/2017 02:37 AM, Matteo Bruni wrote:
> 2017-03-24 23:12 GMT+01:00 Paul Gofman <gofmanp at gmail.com>:
>
>> @@ -697,13 +739,13 @@ static HRESULT parse_preshader(struct d3dx_preshader *pres, unsigned int *ptr, u
>>       for (i = 0; i < pres->ins_count; ++i)
>>       {
>>           for (j = 0; j < pres_op_info[pres->ins[i].op].input_count; ++j)
>> -            update_table_size(pres->regs.table_sizes, pres->ins[i].inputs[j].table,
>> -                    get_reg_offset(pres->ins[i].inputs[j].table,
>> -                    pres->ins[i].inputs[j].offset + pres->ins[i].component_count - 1));
>> +            update_table_size(pres->regs.table_sizes, pres->ins[i].inputs[j].reg.table,
>> +                    get_reg_offset(pres->ins[i].inputs[j].reg.table,
>> +                    pres->ins[i].inputs[j].reg.offset + pres->ins[i].component_count - 1));
>>
>> -        update_table_size(pres->regs.table_sizes, pres->ins[i].output.table,
>> -                get_reg_offset(pres->ins[i].output.table,
>> -                pres->ins[i].output.offset + pres->ins[i].component_count - 1));
>> +        update_table_size(pres->regs.table_sizes, pres->ins[i].output.reg.table,
>> +                get_reg_offset(pres->ins[i].output.reg.table,
>> +                pres->ins[i].output.reg.offset + pres->ins[i].component_count - 1));
> Do these update_table_size() always do the right thing with relative addressing?
> If I'm not mistaken, they should be fine because we only accept
> relative addressing for CONST and IMMED and the former is taken care
> by update_table_sizes_consts() just below while the latter's size is
> set statically just above this chunk. I think that means though that
> we don't need to call update_table_size() on the input arguments.

> If that sounds right to you, you could drop that "for(j)" altogether
> or, probably better, replace the update_table_size() with a check that
> the argument is not out of bounds (probably returning failure if the
> check doesn't pass). That should be a separate patch.
>
     Oh, its a bug actually now, we should not mind absolute offset here 
if relative addressing is on, we know if it is out of bound or not when 
we add index register value only, which can be negative. Yes, it looks 
the only tables where we need to do this update table size are temporary 
register table (for which we don't have any size known in advance) and 
output tables. I would check that and if I am not missing something now 
would leave just update table size where required.
     Otherwise, if to leave the way it is done now, if relative 
addressing is used update_table_size should be called on index register 
table/offset instead of main register.
     Regarding absolute offset check, if the absolute offset (without 
relative addressing) is out of bound fxc does not allow to compile that. 
If to test what happens to native implementation in this case I need to 
update the blob manually. So do you suggest to check if the absolute 
offset to input table is in bound and print a FIXME/fail preshader 
creation in this case?
>> @@ -1088,18 +1130,65 @@ static HRESULT init_set_constants(struct d3dx_const_tab *const_tab, ID3DXConstan
>>       return ret;
>>   }
>>
>> +static double exec_get_reg_value(struct d3dx_regstore *rs, enum pres_reg_tables table, unsigned int offset)
>> +{
>> +    if (!regstore_is_val_set_reg(rs, table, offset / table_info[table].reg_component_count))
>> +        WARN("Using uninitialized input, table %u, offset %u.\n", table, offset);
>> +
>> +    return regstore_get_double(rs, table, offset);
>> +}
>> +
>>   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 offset, base_index, reg_index, table;
>> +
>> +    table = opr->reg.table;
>> +
>> +    if (opr->index_reg.table == PRES_REGTAB_COUNT)
>> +        base_index = 0;
>> +    else
>> +        base_index = lrint(exec_get_reg_value(rs, opr->index_reg.table, opr->index_reg.offset));
>> +
>> +    /* '4' is used instead of reg_component_count, as immediate constants (which have
>> +     *  reg_component_count of 1) are still indexed as 4 values according to the tests.
>> +     */
>> +    offset = base_index * 4 + opr->reg.offset + comp;
> Actually, is there any reason why reg_component_count for
> PRES_REGTAB_IMMED isn't 4? Maybe change it to 4 and in
> parse_preshader() check that const_count is a multiple of 4 (which is
> something we're depending on anyway). That would be a separate patch
> too.
>
> Clue me if I'm missing anything.
Immediate const count is not necessarily the multiple of 4 (it is not 
the case in the present tests). If we set PRES_REGTAB_IMMED count to 4 
it breaks new out of bounds index tests (due to incorrect size to wrap 
to). Besides, immediate constants do not treated as 4-value registers 
anywhere else in preshader code, other than for relative addressing. fxc 
compiler can "pack" constants one after another for there "normal" usage 
(as it was in all our previous tests). So I don't see why we would 
change reg_component_count to 4 for them. But when indexing immed 
constants all the cases I could construct so far index them as 4-value 
packs, even if they are used to index vec3 and just 3 values are used 
from there. I think it is unlikely could be done some other way (if they 
are already undexed in 4-packs in this case), as otherwise each use of 
index to immediate constant would require its own aligment of those 
constants.
>
>> +    reg_index = offset / table_info[table].reg_component_count;
>> +
>> +    if (reg_index >= rs->table_sizes[table])
>> +    {
>> +        unsigned int wrap_size;
>> +
>> +        if (table == PRES_REGTAB_CONST)
>> +        {
>> +            /* As it can be guessed from tests (by iterating base register value and
>> +             * observing the period of result values repeat), offset into floating
>> +             * constant table are wrapped to the nearest power of 2 and not to the
>> +             * actual table size.
>> +             */
>> +            for (wrap_size = 1; wrap_size < rs->table_sizes[table]; wrap_size <<= 1)
>> +                ;
> Please close the comment on the last text line (i.e. "*/" shouldn't be
> on its own line) and maybe remove the part in parentheses which is
> pretty obvious. I like the comment otherwise.
>
> Another thing on this, maybe add a WARN() on wrapped accesses to show
> what offset (or value) is returned in that case? Ignore it if you
> don't think that's useful.
>
Yes, I also think WARN won't hurt in this case.