[Mesa-dev] [PATCH 01/13] i965/fs_cse: Factor out code to create copy instructions

[Pohjolainen, Topi]

On Wed, Apr 01, 2015 at 06:19:12PM -0700, Jason Ekstrand wrote:
> ---
>  src/mesa/drivers/dri/i965/brw_fs_cse.cpp | 75 ++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 37 deletions(-)

Just a few small notes but the logic looks right. Always nice to see things
getting clearer:

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>

> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
> index f2c4098..dd199fa 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
> @@ -183,6 +183,29 @@ instructions_match(fs_inst *a, fs_inst *b, bool *negate)
>            operands_match(a, b, negate);
>  }
>  
> +static fs_inst *
> +create_copy_instr(fs_visitor *v, fs_inst *inst, fs_reg src, bblock_t *block,

You can drop 'block', it isn't used.

> +                  bool negate)
> +{
> +   int written = inst->regs_written;
> +   int dst_width = inst->dst.width / 8;
> +   fs_reg dst = inst->dst;
> +   fs_inst *copy;

Perhaps a separating empty line here? Your choice.

> +   if (written > dst_width) {
> +      fs_reg *sources = ralloc_array(v->mem_ctx, fs_reg, written / dst_width);
> +      for (int i = 0; i < written / dst_width; i++)
> +         sources[i] = offset(src, i);

Other people have instructed me to leave {} out only in the first indentaion
level. I know that original code didn't have them either so this is up to
you.

> +      copy = v->LOAD_PAYLOAD(dst, sources, written / dst_width);
> +   } else {
> +      copy = v->MOV(dst, src);
> +      copy->force_writemask_all = inst->force_writemask_all;
> +      copy->src[0].negate = negate;
> +   }
> +   assert(copy->regs_written == written);
> +
> +   return copy;
> +}
> +
>  bool
>  fs_visitor::opt_cse_local(bblock_t *block)
>  {
> @@ -228,49 +251,27 @@ fs_visitor::opt_cse_local(bblock_t *block)
>              bool no_existing_temp = entry->tmp.file == BAD_FILE;
>              if (no_existing_temp && !entry->generator->dst.is_null()) {
>                 int written = entry->generator->regs_written;
> -               int dst_width = entry->generator->dst.width / 8;
> -               assert(written % dst_width == 0);
> -
> -               fs_reg orig_dst = entry->generator->dst;
> -               fs_reg tmp = fs_reg(GRF, alloc.allocate(written),
> -                                   orig_dst.type, orig_dst.width);
> -               entry->tmp = tmp;
> -               entry->generator->dst = tmp;
> -
> -               fs_inst *copy;
> -               if (written > dst_width) {
> -                  fs_reg *sources = ralloc_array(mem_ctx, fs_reg, written / dst_width);
> -                  for (int i = 0; i < written / dst_width; i++)
> -                     sources[i] = offset(tmp, i);
> -                  copy = LOAD_PAYLOAD(orig_dst, sources, written / dst_width);
> -               } else {
> -                  copy = MOV(orig_dst, tmp);
> -                  copy->force_writemask_all =
> -                     entry->generator->force_writemask_all;
> -               }
> +               assert((written * 8) % entry->generator->dst.width == 0);
> +
> +               entry->tmp = fs_reg(GRF, alloc.allocate(written),
> +                                   entry->generator->dst.type,
> +                                   entry->generator->dst.width);
> +
> +               fs_inst *copy = create_copy_instr(this, entry->generator,
> +                                                 entry->tmp, block, false);
>                 entry->generator->insert_after(block, copy);
> +
> +               entry->generator->dst = entry->tmp;
>              }
>  
>              /* dest <- temp */
>              if (!inst->dst.is_null()) {
> -               int written = inst->regs_written;
> -               int dst_width = inst->dst.width / 8;
> -               assert(written == entry->generator->regs_written);
> -               assert(dst_width == entry->generator->dst.width / 8);
> +               assert(inst->regs_written == entry->generator->regs_written);
> +               assert(inst->dst.width == entry->generator->dst.width);
>                 assert(inst->dst.type == entry->tmp.type);
> -               fs_reg dst = inst->dst;
> -               fs_reg tmp = entry->tmp;
> -               fs_inst *copy;
> -               if (written > dst_width) {
> -                  fs_reg *sources = ralloc_array(mem_ctx, fs_reg, written / dst_width);
> -                  for (int i = 0; i < written / dst_width; i++)
> -                     sources[i] = offset(tmp, i);
> -                  copy = LOAD_PAYLOAD(dst, sources, written / dst_width);
> -               } else {
> -                  copy = MOV(dst, tmp);
> -                  copy->force_writemask_all = inst->force_writemask_all;
> -                  copy->src[0].negate = negate;
> -               }
> +
> +               fs_inst *copy = create_copy_instr(this, inst,
> +                                                 entry->tmp, block, negate);
>                 inst->insert_before(block, copy);
>              }
>  
> -- 
> 2.3.4
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev