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

[Francisco Jerez]

Iago Toral <itoral at igalia.com> writes:

> 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?
>

Right, I think I'd rather have offset() mean the same thing on both the
VEC4 and FS back-ends to avoid confusion (i.e. step by as many scalar
components as the specified SIMD width).

> 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?
>
I don't think an untyped (i.e. based on an offset argument in byte or
32-byte units) variant of offset() would be a particularly compelling
way to achieve the above either, in fact this seems to be the cause of
the rather artificial limitation of the SIMD lowering pass that prevents
it from handling cases where each half doesn't write or read exactly one
register of the instruction.  If you had something like the FS backend's
horiz_offset() you would just get the 4-wide vector of the register for
the group index specified as argument, regardless of the type of the
register.  The current representation of register offsets (as a 32B
multiple) may still get in the way for non-64 bit register types, but
that's not really the SIMD lowering pass' business and can probably be
addressed separately.

> Iago
>
>> > 
>> >     }
>> >  
>> >     reg = retype(reg, type);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160817/f2d8b966/attachment.sig>