[Mesa-dev] [PATCH 16/57] i965/fs: Take into account trailing padding in regs_written() and regs_read().

[Francisco Jerez]

Iago Toral <itoral at igalia.com> writes:

> On Wed, 2016-09-07 at 18:48 -0700, Francisco Jerez wrote:
>> This fixes regs_written() and regs_read() to return a more accurate
>> value when the padding left between components due to a stride value
>> greater than one causes the region bounds given by size_written or
>> size_read to overflow into the next register.  This could become a
>> problem in optimization passes that keep track of dataflow using
>> fixed-size arrays with register granularity, because the overflow
>> register (not actually accessed by the region) may not have been
>> allocated at all which could lead to undefined memory access.
>
> ah, nice catch! I am curious: did you find any cases where this was
> creating a real problem or just something you noticed by inspection? 
>

Yeah, it definitely fixed real problems in a bunch of FP64 tests (I
forgot which ones but I could dig it up if you're interested) which
would have regressed from the next commits that fix regs_written and
regs_read to take into account misalignment -- The only reason this
wasn't a problem before is that we weren't rounding up the register
count correctly in most places...

>> An alternative to this would be to subtract the trailing padding
>> already during the calculation of fs_inst::size_read and
>> ::size_written, but that would break code that currently assumes that
>> ::size_read and _written are whole multiples of the component size,
>> and would be hard to maintain looking forward because size_written is
>> assigned from a bunch of different places.
>
> Yeah, this would be a more problematic solution.
>
>> ---
>>  src/mesa/drivers/dri/i965/brw_ir_fs.h | 22 ++++++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_ir_fs.h
>> b/src/mesa/drivers/dri/i965/brw_ir_fs.h
>> index 4ac9baa..0be67b7 100644
>> --- a/src/mesa/drivers/dri/i965/brw_ir_fs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_ir_fs.h
>> @@ -192,6 +192,20 @@ reg_offset(const fs_reg &r)
>>  }
>>  
>>  /**
>> + * Return the amount of padding in bytes left unused between
>> individual
>> + * components of register \p r due to a (horizontal) stride value
>> greater than
>> + * one, or zero if components are tightly packed in the register
>> file.
>> + */
>> +static inline unsigned
>> +reg_padding(const fs_reg &r)
>> +{
>> +   const unsigned stride = ((r.file != ARF && r.file != FIXED_GRF) ?
>> r.stride :
>> +                            r.hstride == 0 ? 0 :
>> +                            1 << (r.hstride - 1));
>> +   return (MAX2(1, stride) - 1) * type_sz(r.type);
>> +}
>> +
>> +/**
>>   * Return whether the register region starting at \p r and spanning
>> \p dr
>>   * bytes could potentially overlap the register region starting at
>> \p s and
>>   * spanning \p ds bytes.
>> @@ -423,7 +437,9 @@ regs_written(const fs_inst *inst)
>>  {
>>     /* XXX - Take into account register-misaligned offsets correctly.
>> */
>>     assert(inst->dst.file != UNIFORM && inst->dst.file != IMM);
>> -   return DIV_ROUND_UP(inst->size_written, REG_SIZE);
>> +   return DIV_ROUND_UP(inst->size_written -
>> +                       MIN2(inst->size_written, reg_padding(inst-
>> >dst)),
>> +                       REG_SIZE);
>>  }
>>  
>>  /**
>> @@ -438,7 +454,9 @@ regs_read(const fs_inst *inst, unsigned i)
>>     /* XXX - Take into account register-misaligned offsets correctly.
>> */
>>     const unsigned reg_size =
>>        inst->src[i].file == UNIFORM || inst->src[i].file == IMM ? 4 :
>> REG_SIZE;
>> -   return DIV_ROUND_UP(inst->size_read(i), reg_size);
>> +   return DIV_ROUND_UP(inst->size_read(i) -
>> +                       MIN2(inst->size_read(i), reg_padding(inst-
>> >src[i])),
>> +                       reg_size);
>>  }
>>  
>>  #endif
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160909/b93675ad/attachment.sig>