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

Fri May 13 05:22:03 UTC 2016

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 13/05/16 07:11, Samuel Iglesias Gonsálvez wrote:
> 
> 
> 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?
> 

As before, I am going to keep your R-b with these changes.

Sam

> 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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJXNWR7AAoJEH/0ujLxfcNDGM0QAK+E5Y1+fyjzT9yTpZcEFywm
MU4SC5E1ekT8VjfKgAYguMIqvruPoOJGm8QdelBWqGhVClh4gkY+saVVD0cDVN5A
rBeThlP41emZ+CFdTLlrCyKPEPWWKA2g+QNwnufX4gqMis1AgTo5xP55nJh0AIs+
DRE1mt945C44Fg+mLf4Rh6ZCW37qnbvA+Pb+X5e/5G7q4huXWsW8TWh2p8TE1ABW
n0lh5HSYrUi1wNfgcymBhgvnQY1WJaTPsYleWcNMe9XJwPFJUTLeUKw4XZ/ykTV6
CCrHaxlV1c8WonXKoswHO/A0kbaR31YdZNm48QjgKYYpd8tTkW6ptbXqttmLQABy
rQCzlBoj1iDPsX60txg8Hf4NymZjxpozCDvLQzInQuyQiTs46AhSZLJ1dVMsmbSg
PNX6+RF3b+E4m2UrhPAVqDQo90DmxNZv/dvEgc4GXS6QwNTeQSw8IhOUnN0faYSE
uvDYOsiv7N5ewig75UdbiM2v4iZBfFyxwidMn87zBJa/jXUBM49iQMmzmg2s+FZP
HOw5i8kYhtkr/UcVsY2+3fpowkwRTcy9XG5IItv34Dmvr0ZqDTHuX9G5Fx4kNc6w
ooStSoXtl7YcQ/4JaTr/+CBvbq/0Cmfp/ErZOqyhSCz5ehr9Ycalq6g6f7GnOn8N
YsbOsU/j1KMND/t385YR
=b4a3
-----END PGP SIGNATURE-----