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

[Jason Ekstrand]

---
 src/mesa/drivers/dri/i965/brw_fs_cse.cpp | 75 
 1 file changed, 38 insertions(+), 37 deletions(-)

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,
+  bool negate)
+{
+   int written = inst->regs_written;
+   int dst_width = inst->dst.width / 8;
+   fs_reg dst = inst->dst;
+   fs_inst *copy;
+   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);
+  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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 

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

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

[Matt Turner]

On Wed, Apr 1, 2015 at 6:19 PM, Jason Ekstrand  wrote:
> ---
>  src/mesa/drivers/dri/i965/brw_fs_cse.cpp | 75 
> 
>  1 file changed, 38 insertions(+), 37 deletions(-)
>
> 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,

const fs_reg &src

With that and Topi's comments addressed,

Reviewed-by: Matt Turner 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev