[Mesa-dev] [PATCH 5/5] i965/fs: Remove hand-coded 64-bit packing optimizations

[Jason Ekstrand]

On Tue, Feb 14, 2017 at 11:29 PM, Jason Ekstrand <jason at jlekstrand.net>
wrote:

> The optimization in unpack_64 is clearly subsumed with the opt_algebraic
> optimizations in the previous commit.  The pack optimization may not be
> quite handled by opt_algebraic but opt_algebraic should get the really
> bad cases.  Also, it's been broken since it was merged and we've never
> noticed so it must not be doing anything.
>

Ok, that's not quite fair.  It has been broken since it was merged but the
breakage is pretty hard to hit since it requires a NIR register.  However,
this doesn't really cause any regressions either since, prior to patch 1/5,
it was basically never getting triggered.

I did look at some of the shaders mentioned in the bug as being the reason
for this optimization and none of them were hurt by removing it (even after
it was fixed).  I think back-end copy-prop may have just gotten good enough
to make it not needed.


> ---
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 50
> --------------------------------
>  1 file changed, 50 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 91c14eb..9647300 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -1213,61 +1213,11 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> nir_alu_instr *instr)
>        break;
>
>     case nir_op_pack_64_2x32_split:
> -      /* Optimize the common case where we are re-packing a double with
> -       * the result of a previous double unpack. In this case we can take
> the
> -       * 32-bit value to use in the re-pack from the original double and
> bypass
> -       * the unpack operation.
> -       */
> -      for (int i = 0; i < 2; i++) {
> -         if (!instr->src[i].src.is_ssa)
> -            continue;
> -
> -         const nir_instr *parent_instr = instr->src[i].src.ssa->parent_
> instr;
> -         if (parent_instr->type == nir_instr_type_alu)
> -            continue;
> -
> -         const nir_alu_instr *alu_parent = nir_instr_as_alu(parent_instr)
> ;
> -         if (alu_parent->op == nir_op_unpack_64_2x32_split_x ||
> -             alu_parent->op == nir_op_unpack_64_2x32_split_y)
> -            continue;
> -
> -         if (!alu_parent->src[0].src.is_ssa)
> -            continue;
> -
> -         op[i] = get_nir_src(alu_parent->src[0].src);
> -         op[i] = offset(retype(op[i], BRW_REGISTER_TYPE_DF), bld,
> -                        alu_parent->src[0].swizzle[channel]);
> -         if (alu_parent->op == nir_op_unpack_64_2x32_split_y)
> -            op[i] = subscript(op[i], BRW_REGISTER_TYPE_UD, 1);
> -         else
> -            op[i] = subscript(op[i], BRW_REGISTER_TYPE_UD, 0);
> -      }
>        bld.emit(FS_OPCODE_PACK, result, op[0], op[1]);
>        break;
>
>     case nir_op_unpack_64_2x32_split_x:
>     case nir_op_unpack_64_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_64_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_64_2x32_split &&
> -                alu_parent->src[index].src.is_ssa) {
> -               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]);
> -               bld.MOV(result, op[0]);
> -               break;
> -            }
> -         }
> -      }
> -
>        if (instr->op == nir_op_unpack_64_2x32_split_x)
>           bld.MOV(result, subscript(op[0], BRW_REGISTER_TYPE_UD, 0));
>        else
> --
> 2.5.0.400.gff86faf
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170214/d186c899/attachment-0001.html>