[Mesa-dev] [PATCH 32/59] i965/fs: optimize unpack double

[Pohjolainen, Topi]

On Tue, May 03, 2016 at 09:35:42AM +0200, Samuel Iglesias Gons?lvez wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> 
> 
> On 02/05/16 10:38, Pohjolainen, Topi wrote:
> > On Fri, Apr 29, 2016 at 01:29:29PM +0200, Samuel Iglesias Gons?lvez
> > wrote:
> >> From: Iago Toral Quiroga <itoral at igalia.com>
> >> 
> >> When we are actually unpacking from a double that we have
> >> previously packed from its 32-bit components we can bypass the
> >> pack operation and source from its arguments directly. --- 
> >> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 28
> >> ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4
> >> deletions(-)
> >> 
> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> >> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp index 4332575..0ce25a0
> >> 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp +++
> >> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp @@ -1118,12 +1118,32
> >> @@ fs_visitor::nir_emit_alu(const fs_builder &bld, nir_alu_instr
> >> *instr) break;
> >> 
> >> case nir_op_unpack_double_2x32_split_x: -      bld.MOV(result,
> >> stride(retype(op[0], BRW_REGISTER_TYPE_UD), 2)); -      break; +
> >> case nir_op_unpack_double_2x32_split_y: { +      /* Optimize the
> >> common case where we are unpacking from a double we have +
> >> * previously packed. In this case we can just bypass the pack
> >> operation +       * and source directly from its arguments. +
> >> */ +      unsigned index = (instr->op ==
> >> nir_op_unpack_double_2x32_split_x) ? 0 : 1; +      if
> >> (instr->src[0].src.is_ssa) { +         nir_instr *parent_instr =
> >> instr->src[0].src.ssa->parent_instr; +         if
> >> (parent_instr->type == nir_instr_type_alu) { +
> >> nir_alu_instr *alu_parent = nir_instr_as_alu(parent_instr); +
> >> if (alu_parent->op == nir_op_pack_double_2x32_split) { +
> >> op[0] = retype(get_nir_src(alu_parent->src[index].src), +
> >> BRW_REGISTER_TYPE_UD); +               op[0] = offset(op[0], bld,
> >> alu_parent->src[index].swizzle[channel]);
> > 
> > Overflowing line, I tried further down hot it looks with less
> > indentation. I don't know if it was better or worse.
> > 
> 
> Why not just:
> 
> op[0] =
>    offset(op[0], bld, alu_parent->src[index].swizzle[channel]);
> 
> I think that's easier and doesn't complicate the rest of the code.

Sure, which ever way you feel best.