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?
> +{
> + 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.
> + 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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
