On Fri, 2016-09-09 at 13:30 -0700, Francisco Jerez wrote: > Iago Toral <ito...@igalia.com> writes: > > > > > On Wed, 2016-09-07 at 18:48 -0700, Francisco Jerez wrote: > > > > > > This was dropping 'inst->dst.offset' on the floor. Nothing in > > > the > > > code above seems to guarantee that it's zero and in that case the > > > offset of the register being coalesced into wouldn't be taken > > > into > > > account while rewriting the generating instruction. > > > --- > > > src/mesa/drivers/dri/i965/brw_fs.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp > > > b/src/mesa/drivers/dri/i965/brw_fs.cpp > > > index 98da06c..5b7ca98 100644 > > > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp > > > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp > > > @@ -2821,7 +2821,7 @@ fs_visitor::compute_to_mrf() > > > } > > > > > > scan_inst->dst.file = MRF; > > > - scan_inst->dst.offset %= REG_SIZE; > > > + scan_inst->dst.offset = inst->dst.offset + > > > scan_inst- > > > > > > > > dst.offset % REG_SIZE; > > So if we had something like this: > > > > 0: mov(4) r1.4:F r0.0:F > > 1: mov(4) m1:F r1.4:F > > > > This pass is trying to get us: > > > > mov(4) m1:F r0.0:F > > > Yeah, that's what it should be doing, but the current code (already > before this series) would give you: > > > > > mov(4) m1.4:F r0.0:F > which is pretty bogus. > > > > > In that case, the offset into the destination of the instruction we > > are propagating from should not affect the offset into the MRF we > > are writing to. Or maybe I am missing what is going on here :) > Yeah, that's exactly the problem fixed in the next patch. Note that > this commit doesn't actually introduce the bug, it's preserving the > buggy behavior of preexisting code, which happens to become more > obvious > due to the explicit 'scan_inst->dst.offset % REG_SIZE' term instead > of > the '%=' operator.
Aha, ok, I missed the fact that the bug was already there. In that case yes, it probably makes sense to keep both patches as they are. > > > > > > > > scan_inst->saturate |= inst->saturate; > > > if (!regs_left) > > > break; _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
