-----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 <[email protected]> writes: >> >>> From: Iago Toral Quiroga <[email protected]> >>> >>> 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 <[email protected]> >>> Reviewed-by: Francisco Jerez <[email protected]> --- >>> .../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 [email protected] >>> 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----- _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
