[Mesa-dev] [PATCH 28/57] i965/fs: Take into account copy register offset during compute-to-mrf.

[Iago Toral]

On Fri, 2016-09-09 at 13:30 -0700, Francisco Jerez wrote:
> Iago Toral <itoral at 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;