<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 3, 2016 at 5:21 AM, Samuel Iglesias Gonsálvez <span dir="ltr"><<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Iago Toral Quiroga <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>><br>
<br>
We were not accounting for reg_suboffset in the check for the start<br>
of the region. This meant that would allow copy-propagation even if<br>
the dst wrote to sub_regoffset 4 and our source read from<br>
sub_regoffset 0, which is not correct. This was observed in fp64 code,<br>
since there we use reg_suboffset to select the high 32-bit of a double.<br></blockquote><div><br></div><div>I'm not sure if copy-prop can handle the case where you write to sub_regoffset != 0 anyway. Maybe we should just bail on that?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, fs_reg::regs_read() already takes the stride into account, so we<br>
should not multiply its result by the stride again. This was making<br>
copy-propagation fail to copy-propagate cases that would otherwise be<br>
safe to copy-propagate. Again, this was observed in fp64 code, since<br>
there we use stride > 1 often.<br>
<br>
Incidentally, these fixes open up more possibilities for copy propagation<br>
which uncovered new bugs in copy-propagation. The folowing patches address<br>
each of these new issues.<br>
---<br>
.../drivers/dri/i965/brw_fs_copy_propagation.cpp | 21 +++++++++++++--------<br>
1 file changed, 13 insertions(+), 8 deletions(-)<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp<br>
index 5fae10f..23df877 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp<br>
+++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp<br>
@@ -329,6 +329,15 @@ can_take_stride(fs_inst *inst, unsigned arg, unsigned stride,<br>
return true;<br>
}<br>
<br>
+static inline bool<br>
+region_match(fs_reg src, unsigned regs_read,<br>
+ fs_reg dst, unsigned regs_written)<br></blockquote><div><br></div><div>I don't like the name. It should say something about containment with perhaps a comment to go along with it. Should we assert that they have the same file and number?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+{<br>
+ return src.reg_offset >= dst.reg_offset &&<br>
+ (src.reg_offset + regs_read) <= (dst.reg_offset + regs_written) &&<br>
+ src.subreg_offset >= dst.subreg_offset;<br>
+}<br>
+<br>
bool<br>
fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)<br>
{<br>
@@ -351,10 +360,8 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)<br>
/* Bail if inst is reading a range that isn't contained in the range<br>
* that entry is writing.<br>
*/<br>
- if (inst->src[arg].reg_offset < entry->dst.reg_offset ||<br>
- (inst->src[arg].reg_offset * 32 + inst->src[arg].subreg_offset +<br>
- inst->regs_read(arg) * inst->src[arg].stride * 32) ><br>
- (entry->dst.reg_offset + entry->regs_written) * 32)<br>
+ if (!region_match(inst->src[arg], inst->regs_read(arg),<br>
+ entry->dst, entry->regs_written))<br>
return false;<br>
<br>
/* we can't generally copy-propagate UD negations because we<br>
@@ -554,10 +561,8 @@ fs_visitor::try_constant_propagate(fs_inst *inst, acp_entry *entry)<br>
/* Bail if inst is reading a range that isn't contained in the range<br>
* that entry is writing.<br>
*/<br>
- if (inst->src[i].reg_offset < entry->dst.reg_offset ||<br>
- (inst->src[i].reg_offset * 32 + inst->src[i].subreg_offset +<br>
- inst->regs_read(i) * inst->src[i].stride * 32) ><br>
- (entry->dst.reg_offset + entry->regs_written) * 32)<br>
+ if (!region_match(inst->src[i], inst->regs_read(i),<br>
+ entry->dst, entry->regs_written))<br>
continue;<br>
<br>
fs_reg val = entry->src;<br>
<span class="HOEnZb"><font color="#888888">--<br>
2.5.0<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div><br></div></div>