[Mesa-dev] [PATCH v2 09/30] i965/fs: fix copy/constant propagation regioning checks

[Samuel Iglesias Gonsálvez]

On 13/05/16 02:42, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
> 
>> From: Iago Toral Quiroga <itoral at igalia.com>
>>
>> We were not accounting for subreg_offset in the check for the start
>> of the region.
>>
>> Also, fs_reg::regs_read() already takes the stride into account, so we
>> should not multiply its result by the stride again. This was making
>> copy-propagation fail to copy-propagate cases that would otherwise be
>> safe to copy-propagate. Again, this was observed in fp64 code, since
>> there we use stride > 1 often.
>>
>> v2 (Sam):
>> - Rename function and add comment (Jason, Curro).
>> - Assert that register files and number are the same (Jason).
>> - Fix code to take into account the assumption that src.subreg_offset
>> is strictly less than the reg_offset unit (Curro).
>> - Don't pass the registers by value to the function, use
>> 'const fs_reg &' instead (Curro).
>> - Remove obsolete comment in the commit log (Curro).
>>
>> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
>> ---
>>  .../drivers/dri/i965/brw_fs_copy_propagation.cpp   | 28 +++++++++++++++-------
>>  1 file changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>> index 4791894..084d315 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>> @@ -330,6 +330,22 @@ can_take_stride(fs_inst *inst, unsigned arg, unsigned stride,
>>     return true;
>>  }
>>  
>> +/**
>> + * Check that the register region read by src [src.reg_offset,
>> + * src.reg_offset + regs_read] is contained inside the register
>> + * region written by dst [dst.reg_offset, dst.reg_offset + regs_written]
>> + * Both src and dst must have the same register number and file.
>> + */
>> +static inline bool
>> +regions_contained_in(const fs_reg &src, unsigned regs_read,
>> +                     const fs_reg &dst, unsigned regs_written)
> 
> region*s*?  Aren't you checking whether one region is contained inside
> another?
> 

Right, I am going to rename it.

>> +{
>> +   assert(src.file == dst.file && src.nr == dst.nr);
> 
> I don't like raising an assertion failure here even though you could
> trivially make the function behave properly in the 'src.file != dst.file ||
> src.nr != dst.nr' case by adding 'src.file == dst.file && src.nr ==
> dst.nr' (two regions from different files are necessarily
> not contained inside each other) to the expression below.  That would
> actually make the function more useful and let you get rid of the
> explicit fs_reg::nr checks in try_constant_propagate() and
> try_copy_propagate().
> 
> Either way feel free to ignore my suggestion for the moment if you'd
> rather do it as a follow-up clean-up, the code seems functionally
> correct as-is AFAICT.
> 

You are right. I will add the condition to the return and remove the assert.

Does it get your R-b with these changes?

Sam


>> +   return (src.reg_offset * REG_SIZE + src.subreg_offset >=
>> +           dst.reg_offset * REG_SIZE + dst.subreg_offset) &&
>> +      src.reg_offset + regs_read <= dst.reg_offset + regs_written;
>> +}
>> +
>>  bool
>>  fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)
>>  {
>> @@ -352,10 +368,8 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)
>>     /* Bail if inst is reading a range that isn't contained in the range
>>      * that entry is writing.
>>      */
>> -   if (inst->src[arg].reg_offset < entry->dst.reg_offset ||
>> -       (inst->src[arg].reg_offset * 32 + inst->src[arg].subreg_offset +
>> -        inst->regs_read(arg) * inst->src[arg].stride * 32) >
>> -       (entry->dst.reg_offset + entry->regs_written) * 32)
>> +   if (!regions_contained_in(inst->src[arg], inst->regs_read(arg),
>> +                             entry->dst, entry->regs_written))
>>        return false;
>>  
>>     /* we can't generally copy-propagate UD negations because we
>> @@ -520,10 +534,8 @@ fs_visitor::try_constant_propagate(fs_inst *inst, acp_entry *entry)
>>        /* Bail if inst is reading a range that isn't contained in the range
>>         * that entry is writing.
>>         */
>> -      if (inst->src[i].reg_offset < entry->dst.reg_offset ||
>> -          (inst->src[i].reg_offset * 32 + inst->src[i].subreg_offset +
>> -           inst->regs_read(i) * inst->src[i].stride * 32) >
>> -          (entry->dst.reg_offset + entry->regs_written) * 32)
>> +      if (!regions_contained_in(inst->src[i], inst->regs_read(i),
>> +                                entry->dst, entry->regs_written))
>>           continue;
>>  
>>        fs_reg val = entry->src;
>> -- 
>> 2.5.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev