[Mesa-dev] [PATCH 25/95] i965/vec4: fix base offset for nir_registers with doubles

[Iago Toral]

On Tue, 2016-08-02 at 18:40 -0700, Francisco Jerez wrote:
> Iago Toral Quiroga <itoral at igalia.com> writes:
> 
> > 
> > ---
> >  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> > b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> > index cf35f2e..fde7b60 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> > @@ -280,7 +280,8 @@ vec4_visitor::get_nir_dest(const nir_dest
> > &dest)
> >        nir_ssa_values[dest.ssa.index] = dst;
> >        return dst;
> >     } else {
> > -      return dst_reg_for_nir_reg(this, dest.reg.reg,
> > dest.reg.base_offset,
> > +      unsigned base_offset = dest.reg.base_offset * dest.reg.reg-
> > >bit_size / 32;
> > +      return dst_reg_for_nir_reg(this, dest.reg.reg, base_offset,
> >                                   dest.reg.indirect);
> >     }
> >  }
> > @@ -308,8 +309,9 @@ vec4_visitor::get_nir_src(const nir_src &src,
> > enum brw_reg_type type,
> >        reg = nir_ssa_values[src.ssa->index];
> >     }
> >     else {
> > -     reg = dst_reg_for_nir_reg(this, src.reg.reg,
> > src.reg.base_offset,
> > -                               src.reg.indirect);
> > +      unsigned base_offset = src.reg.base_offset * src.reg.reg-
> > >bit_size / 32;
> > +      reg = dst_reg_for_nir_reg(this, src.reg.reg, base_offset,
> > +                                src.reg.indirect);
> I think this wouldn't have been necessary if you had fixed the
> offset()
> helper to take into account the register type (as it does in the FS
> back-end)?

Yes, you are right.

I found it convenient that offset() didn't consider the type because
that gave us a bit more flexibility to offset into the second half of a
SIMD4x2 dvecN simply by doing offset(reg, 1). If offset takes the type
into account we lose that ability because for a DF register the minimum
we could offset with a delta=1 would be 2 SIMD8 registers (a full
SIMD4x2 dvec4). We can still avoid calling offset() in these cases and
just increase reg_offset manually by 1 instead, I understand that you
prefer this?

Alternatively, we could also have typed and untyped versions of the
offset function so we can choose the one we need depending on the case,
but maybe that would be more confusing?

Iago

> > 
> >     }
> >  
> >     reg = retype(reg, type);