[Mesa-dev,10/95] i965/vec4: handle 32 and 64 bit channels in liveness analysis

Submitted by Iago Toral Quiroga on July 19, 2016, 10:40 a.m.

Details

Message ID 1468924892-6910-11-git-send-email-itoral@igalia.com
State New
Headers show
Series "i965 Haswell ARB_gpu_shader_fp64 / OpenGL 4.0" ( rev: 2 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga July 19, 2016, 10:40 a.m.
From: "Juan A. Suarez Romero" <jasuarez@igalia.com>

Our current data flow analysis does not take into account that channels
on 64-bit operands are 64-bit. This is a problem when the same register
is accessed using both 64-bit and 32-bit channels. This is very common
in operations where we need to access 64-bit data in 32-bit chunks,
such as the double packing and packing operations.

This patch changes the analysis by checking the bits that each source
or destination datatype needs. Actually, rather than bits, we use
blocks of 32bits, which is the minimum channel size.

Because a vgrf can contain a dvec4 (256 bits), we reserve 8
32-bit blocks to map the channels.
---
 .../dri/i965/brw_vec4_dead_code_eliminate.cpp      | 30 +++++++++++++-----
 .../drivers/dri/i965/brw_vec4_live_variables.cpp   | 36 +++++++++++++++++-----
 .../drivers/dri/i965/brw_vec4_live_variables.h     |  7 +++--
 3 files changed, 55 insertions(+), 18 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
index c643212..1f7cd49 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
@@ -59,10 +59,16 @@  vec4_visitor::dead_code_eliminate()
             bool result_live[4] = { false };
 
             if (inst->dst.file == VGRF) {
+               const unsigned offs = type_sz(inst->dst.type) == 8 ?
+                  1 : (inst->exec_size == 4 ? 0 : 4);
                for (unsigned i = 0; i < inst->regs_written; i++) {
-                  for (int c = 0; c < 4; c++)
-                     result_live[c] |= BITSET_TEST(
-                        live, var_from_reg(alloc, offset(inst->dst, i), c));
+                  for (int c = 0; c < 4; c++) {
+                     const unsigned v =
+                        var_from_reg(alloc, offset(inst->dst, i), c);
+                     result_live[c] |= BITSET_TEST(live, v);
+                     if (offs)
+                        result_live[c] |= BITSET_TEST(live, v + offs);
+                  }
                }
             } else {
                for (unsigned c = 0; c < 4; c++)
@@ -110,11 +116,16 @@  vec4_visitor::dead_code_eliminate()
          }
 
          if (inst->dst.file == VGRF && !inst->predicate) {
+            const unsigned offs = type_sz(inst->dst.type) == 8 ?
+               1 : (inst->exec_size == 4 ? 0 : 4);
             for (unsigned i = 0; i < inst->regs_written; i++) {
                for (int c = 0; c < 4; c++) {
                   if (inst->dst.writemask & (1 << c)) {
-                     BITSET_CLEAR(live, var_from_reg(alloc,
-                                                     offset(inst->dst, i), c));
+                     const unsigned v =
+                        var_from_reg(alloc, offset(inst->dst, i), c);
+                     BITSET_CLEAR(live, v);
+                     if (offs)
+                        BITSET_CLEAR(live, v + offs);
                   }
                }
             }
@@ -132,10 +143,15 @@  vec4_visitor::dead_code_eliminate()
 
          for (int i = 0; i < 3; i++) {
             if (inst->src[i].file == VGRF) {
+               const unsigned offs = type_sz(inst->src[i].type) == 8 ?
+                  1 : (inst->exec_size == 4 ? 0 : 4);
                for (unsigned j = 0; j < inst->regs_read(i); j++) {
                   for (int c = 0; c < 4; c++) {
-                     BITSET_SET(live, var_from_reg(alloc,
-                                                   offset(inst->src[i], j), c));
+                     const unsigned v =
+                        var_from_reg(alloc, offset(inst->src[i], j), c);
+                     BITSET_SET(live, v);
+                     if (offs)
+                        BITSET_SET(live, v + offs);
                   }
                }
             }
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
index 57d5fbb..bfc5e44 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
@@ -76,12 +76,16 @@  vec4_live_variables::setup_def_use()
 	 /* Set use[] for this instruction */
 	 for (unsigned int i = 0; i < 3; i++) {
 	    if (inst->src[i].file == VGRF) {
+               const unsigned offs = type_sz(inst->dst.type) == 8 ?
+                  1 : (inst->exec_size == 4 ? 0 : 4);
                for (unsigned j = 0; j < inst->regs_read(i); j++) {
                   for (int c = 0; c < 4; c++) {
                      const unsigned v =
                         var_from_reg(alloc, offset(inst->src[i], j), c);
                      if (!BITSET_TEST(bd->def, v))
                         BITSET_SET(bd->use, v);
+                     if (offs && !BITSET_TEST(bd->def, v + offs))
+                        BITSET_SET(bd->use, v + offs);
                   }
                }
 	    }
@@ -99,6 +103,8 @@  vec4_live_variables::setup_def_use()
 	  */
 	 if (inst->dst.file == VGRF &&
 	     (!inst->predicate || inst->opcode == BRW_OPCODE_SEL)) {
+            const unsigned offs = type_sz(inst->dst.type) == 8 ?
+               1 : (inst->exec_size == 4 ? 0 : 4);
             for (unsigned i = 0; i < inst->regs_written; i++) {
                for (int c = 0; c < 4; c++) {
                   if (inst->dst.writemask & (1 << c)) {
@@ -106,6 +112,8 @@  vec4_live_variables::setup_def_use()
                         var_from_reg(alloc, offset(inst->dst, i), c);
                      if (!BITSET_TEST(bd->use, v))
                         BITSET_SET(bd->def, v);
+                     if (offs && !BITSET_TEST(bd->use, v + offs))
+                        BITSET_SET(bd->def, v + offs);
                   }
                }
             }
@@ -188,7 +196,7 @@  vec4_live_variables::vec4_live_variables(const simple_allocator &alloc,
 {
    mem_ctx = ralloc_context(NULL);
 
-   num_vars = alloc.total_size * 4;
+   num_vars = alloc.total_size * 8;
    block_data = rzalloc_array(mem_ctx, struct block_data, cfg->num_blocks);
 
    bitset_words = BITSET_WORDS(num_vars);
@@ -238,14 +246,14 @@  vec4_visitor::calculate_live_intervals()
    if (this->live_intervals)
       return;
 
-   int *start = ralloc_array(mem_ctx, int, this->alloc.total_size * 4);
-   int *end = ralloc_array(mem_ctx, int, this->alloc.total_size * 4);
+   int *start = ralloc_array(mem_ctx, int, this->alloc.total_size * 8);
+   int *end = ralloc_array(mem_ctx, int, this->alloc.total_size * 8);
    ralloc_free(this->virtual_grf_start);
    ralloc_free(this->virtual_grf_end);
    this->virtual_grf_start = start;
    this->virtual_grf_end = end;
 
-   for (unsigned i = 0; i < this->alloc.total_size * 4; i++) {
+   for (unsigned i = 0; i < this->alloc.total_size * 8; i++) {
       start[i] = MAX_INSTRUCTION;
       end[i] = -1;
    }
@@ -257,18 +265,26 @@  vec4_visitor::calculate_live_intervals()
    foreach_block_and_inst(block, vec4_instruction, inst, cfg) {
       for (unsigned int i = 0; i < 3; i++) {
 	 if (inst->src[i].file == VGRF) {
+            const unsigned offs = type_sz(inst->dst.type) == 8 ?
+               1 : (inst->exec_size == 4 ? 0 : 4);
             for (unsigned j = 0; j < inst->regs_read(i); j++) {
                for (int c = 0; c < 4; c++) {
                   const unsigned v =
                      var_from_reg(alloc, offset(inst->src[i], j), c);
                   start[v] = MIN2(start[v], ip);
                   end[v] = ip;
+                  if (offs) {
+                     start[v + offs] = MIN2(start[v + offs], ip);
+                     end[v + offs] = ip;
+                  }
                }
             }
 	 }
       }
 
       if (inst->dst.file == VGRF) {
+         const unsigned offs = type_sz(inst->dst.type) == 8 ?
+            1 : (inst->exec_size == 4 ? 0 : 4);
          for (unsigned i = 0; i < inst->regs_written; i++) {
             for (int c = 0; c < 4; c++) {
                if (inst->dst.writemask & (1 << c)) {
@@ -276,6 +292,10 @@  vec4_visitor::calculate_live_intervals()
                      var_from_reg(alloc, offset(inst->dst, i), c);
                   start[v] = MIN2(start[v], ip);
                   end[v] = ip;
+                  if (offs) {
+                     start[v + offs] = MIN2(start[v + offs], ip);
+                     end[v + offs] = ip;
+                  }
                }
             }
          }
@@ -340,8 +360,8 @@  vec4_visitor::var_range_end(unsigned v, unsigned n) const
 bool
 vec4_visitor::virtual_grf_interferes(int a, int b)
 {
-   return !((var_range_end(4 * alloc.offsets[a], 4 * alloc.sizes[a]) <=
-             var_range_start(4 * alloc.offsets[b], 4 * alloc.sizes[b])) ||
-            (var_range_end(4 * alloc.offsets[b], 4 * alloc.sizes[b]) <=
-             var_range_start(4 * alloc.offsets[a], 4 * alloc.sizes[a])));
+   return !((var_range_end(8 * alloc.offsets[a], 8 * alloc.sizes[a]) <=
+             var_range_start(8 * alloc.offsets[b], 8 * alloc.sizes[b])) ||
+            (var_range_end(8 * alloc.offsets[b], 8 * alloc.sizes[b]) <=
+             var_range_start(8 * alloc.offsets[a], 8 * alloc.sizes[a])));
 }
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
index 12d281e..cdbd7dc 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
+++ b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
@@ -84,8 +84,8 @@  var_from_reg(const simple_allocator &alloc, const src_reg &reg,
 {
    assert(reg.file == VGRF && reg.nr < alloc.count &&
           reg.reg_offset < alloc.sizes[reg.nr] && c < 4);
-   return (4 * (alloc.offsets[reg.nr] + reg.reg_offset) +
-           BRW_GET_SWZ(reg.swizzle, c));
+   return (8 * (alloc.offsets[reg.nr] + reg.reg_offset) +
+           BRW_GET_SWZ(reg.swizzle, c) * MAX2(1, type_sz(reg.type) / 4));
 }
 
 inline unsigned
@@ -94,7 +94,8 @@  var_from_reg(const simple_allocator &alloc, const dst_reg &reg,
 {
    assert(reg.file == VGRF && reg.nr < alloc.count &&
           reg.reg_offset < alloc.sizes[reg.nr] && c < 4);
-   return 4 * (alloc.offsets[reg.nr] + reg.reg_offset) + c;
+   return 8 * (alloc.offsets[reg.nr] + reg.reg_offset) +
+          c * MAX2(1, type_sz(reg.type) / 4);
 }
 
 } /* namespace brw */

Comments

This patch should not be so early in the series. It uses the execsize
information in the IR which is not available until patch 38, so it
should really go after that.

Iago

On Tue, 2016-07-19 at 12:40 +0200, Iago Toral Quiroga wrote:
> From: "Juan A. Suarez Romero" <jasuarez@igalia.com>
> 
> Our current data flow analysis does not take into account that
> channels
> on 64-bit operands are 64-bit. This is a problem when the same
> register
> is accessed using both 64-bit and 32-bit channels. This is very
> common
> in operations where we need to access 64-bit data in 32-bit chunks,
> such as the double packing and packing operations.
> 
> This patch changes the analysis by checking the bits that each source
> or destination datatype needs. Actually, rather than bits, we use
> blocks of 32bits, which is the minimum channel size.
> 
> Because a vgrf can contain a dvec4 (256 bits), we reserve 8
> 32-bit blocks to map the channels.
> ---
>  .../dri/i965/brw_vec4_dead_code_eliminate.cpp      | 30
> +++++++++++++-----
>  .../drivers/dri/i965/brw_vec4_live_variables.cpp   | 36
> +++++++++++++++++-----
>  .../drivers/dri/i965/brw_vec4_live_variables.h     |  7 +++--
>  3 files changed, 55 insertions(+), 18 deletions(-)
> 
> diff --git
> a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> index c643212..1f7cd49 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> @@ -59,10 +59,16 @@ vec4_visitor::dead_code_eliminate()
>              bool result_live[4] = { false };
>  
>              if (inst->dst.file == VGRF) {
> +               const unsigned offs = type_sz(inst->dst.type) == 8 ?
> +                  1 : (inst->exec_size == 4 ? 0 : 4);
>                 for (unsigned i = 0; i < inst->regs_written; i++) {
> -                  for (int c = 0; c < 4; c++)
> -                     result_live[c] |= BITSET_TEST(
> -                        live, var_from_reg(alloc, offset(inst->dst,
> i), c));
> +                  for (int c = 0; c < 4; c++) {
> +                     const unsigned v =
> +                        var_from_reg(alloc, offset(inst->dst, i),
> c);
> +                     result_live[c] |= BITSET_TEST(live, v);
> +                     if (offs)
> +                        result_live[c] |= BITSET_TEST(live, v +
> offs);
> +                  }
>                 }
>              } else {
>                 for (unsigned c = 0; c < 4; c++)
> @@ -110,11 +116,16 @@ vec4_visitor::dead_code_eliminate()
>           }
>  
>           if (inst->dst.file == VGRF && !inst->predicate) {
> +            const unsigned offs = type_sz(inst->dst.type) == 8 ?
> +               1 : (inst->exec_size == 4 ? 0 : 4);
>              for (unsigned i = 0; i < inst->regs_written; i++) {
>                 for (int c = 0; c < 4; c++) {
>                    if (inst->dst.writemask & (1 << c)) {
> -                     BITSET_CLEAR(live, var_from_reg(alloc,
> -                                                     offset(inst-
> >dst, i), c));
> +                     const unsigned v =
> +                        var_from_reg(alloc, offset(inst->dst, i),
> c);
> +                     BITSET_CLEAR(live, v);
> +                     if (offs)
> +                        BITSET_CLEAR(live, v + offs);
>                    }
>                 }
>              }
> @@ -132,10 +143,15 @@ vec4_visitor::dead_code_eliminate()
>  
>           for (int i = 0; i < 3; i++) {
>              if (inst->src[i].file == VGRF) {
> +               const unsigned offs = type_sz(inst->src[i].type) == 8
> ?
> +                  1 : (inst->exec_size == 4 ? 0 : 4);
>                 for (unsigned j = 0; j < inst->regs_read(i); j++) {
>                    for (int c = 0; c < 4; c++) {
> -                     BITSET_SET(live, var_from_reg(alloc,
> -                                                   offset(inst-
> >src[i], j), c));
> +                     const unsigned v =
> +                        var_from_reg(alloc, offset(inst->src[i], j),
> c);
> +                     BITSET_SET(live, v);
> +                     if (offs)
> +                        BITSET_SET(live, v + offs);
>                    }
>                 }
>              }
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> index 57d5fbb..bfc5e44 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> @@ -76,12 +76,16 @@ vec4_live_variables::setup_def_use()
>  	 /* Set use[] for this instruction */
>  	 for (unsigned int i = 0; i < 3; i++) {
>  	    if (inst->src[i].file == VGRF) {
> +               const unsigned offs = type_sz(inst->dst.type) == 8 ?
> +                  1 : (inst->exec_size == 4 ? 0 : 4);
>                 for (unsigned j = 0; j < inst->regs_read(i); j++) {
>                    for (int c = 0; c < 4; c++) {
>                       const unsigned v =
>                          var_from_reg(alloc, offset(inst->src[i], j),
> c);
>                       if (!BITSET_TEST(bd->def, v))
>                          BITSET_SET(bd->use, v);
> +                     if (offs && !BITSET_TEST(bd->def, v + offs))
> +                        BITSET_SET(bd->use, v + offs);
>                    }
>                 }
>  	    }
> @@ -99,6 +103,8 @@ vec4_live_variables::setup_def_use()
>  	  */
>  	 if (inst->dst.file == VGRF &&
>  	     (!inst->predicate || inst->opcode == BRW_OPCODE_SEL)) {
> +            const unsigned offs = type_sz(inst->dst.type) == 8 ?
> +               1 : (inst->exec_size == 4 ? 0 : 4);
>              for (unsigned i = 0; i < inst->regs_written; i++) {
>                 for (int c = 0; c < 4; c++) {
>                    if (inst->dst.writemask & (1 << c)) {
> @@ -106,6 +112,8 @@ vec4_live_variables::setup_def_use()
>                          var_from_reg(alloc, offset(inst->dst, i),
> c);
>                       if (!BITSET_TEST(bd->use, v))
>                          BITSET_SET(bd->def, v);
> +                     if (offs && !BITSET_TEST(bd->use, v + offs))
> +                        BITSET_SET(bd->def, v + offs);
>                    }
>                 }
>              }
> @@ -188,7 +196,7 @@ vec4_live_variables::vec4_live_variables(const
> simple_allocator &alloc,
>  {
>     mem_ctx = ralloc_context(NULL);
>  
> -   num_vars = alloc.total_size * 4;
> +   num_vars = alloc.total_size * 8;
>     block_data = rzalloc_array(mem_ctx, struct block_data, cfg-
> >num_blocks);
>  
>     bitset_words = BITSET_WORDS(num_vars);
> @@ -238,14 +246,14 @@ vec4_visitor::calculate_live_intervals()
>     if (this->live_intervals)
>        return;
>  
> -   int *start = ralloc_array(mem_ctx, int, this->alloc.total_size *
> 4);
> -   int *end = ralloc_array(mem_ctx, int, this->alloc.total_size *
> 4);
> +   int *start = ralloc_array(mem_ctx, int, this->alloc.total_size *
> 8);
> +   int *end = ralloc_array(mem_ctx, int, this->alloc.total_size *
> 8);
>     ralloc_free(this->virtual_grf_start);
>     ralloc_free(this->virtual_grf_end);
>     this->virtual_grf_start = start;
>     this->virtual_grf_end = end;
>  
> -   for (unsigned i = 0; i < this->alloc.total_size * 4; i++) {
> +   for (unsigned i = 0; i < this->alloc.total_size * 8; i++) {
>        start[i] = MAX_INSTRUCTION;
>        end[i] = -1;
>     }
> @@ -257,18 +265,26 @@ vec4_visitor::calculate_live_intervals()
>     foreach_block_and_inst(block, vec4_instruction, inst, cfg) {
>        for (unsigned int i = 0; i < 3; i++) {
>  	 if (inst->src[i].file == VGRF) {
> +            const unsigned offs = type_sz(inst->dst.type) == 8 ?
> +               1 : (inst->exec_size == 4 ? 0 : 4);
>              for (unsigned j = 0; j < inst->regs_read(i); j++) {
>                 for (int c = 0; c < 4; c++) {
>                    const unsigned v =
>                       var_from_reg(alloc, offset(inst->src[i], j),
> c);
>                    start[v] = MIN2(start[v], ip);
>                    end[v] = ip;
> +                  if (offs) {
> +                     start[v + offs] = MIN2(start[v + offs], ip);
> +                     end[v + offs] = ip;
> +                  }
>                 }
>              }
>  	 }
>        }
>  
>        if (inst->dst.file == VGRF) {
> +         const unsigned offs = type_sz(inst->dst.type) == 8 ?
> +            1 : (inst->exec_size == 4 ? 0 : 4);
>           for (unsigned i = 0; i < inst->regs_written; i++) {
>              for (int c = 0; c < 4; c++) {
>                 if (inst->dst.writemask & (1 << c)) {
> @@ -276,6 +292,10 @@ vec4_visitor::calculate_live_intervals()
>                       var_from_reg(alloc, offset(inst->dst, i), c);
>                    start[v] = MIN2(start[v], ip);
>                    end[v] = ip;
> +                  if (offs) {
> +                     start[v + offs] = MIN2(start[v + offs], ip);
> +                     end[v + offs] = ip;
> +                  }
>                 }
>              }
>           }
> @@ -340,8 +360,8 @@ vec4_visitor::var_range_end(unsigned v, unsigned
> n) const
>  bool
>  vec4_visitor::virtual_grf_interferes(int a, int b)
>  {
> -   return !((var_range_end(4 * alloc.offsets[a], 4 * alloc.sizes[a])
> <=
> -             var_range_start(4 * alloc.offsets[b], 4 *
> alloc.sizes[b])) ||
> -            (var_range_end(4 * alloc.offsets[b], 4 * alloc.sizes[b])
> <=
> -             var_range_start(4 * alloc.offsets[a], 4 *
> alloc.sizes[a])));
> +   return !((var_range_end(8 * alloc.offsets[a], 8 * alloc.sizes[a])
> <=
> +             var_range_start(8 * alloc.offsets[b], 8 *
> alloc.sizes[b])) ||
> +            (var_range_end(8 * alloc.offsets[b], 8 * alloc.sizes[b])
> <=
> +             var_range_start(8 * alloc.offsets[a], 8 *
> alloc.sizes[a])));
>  }
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
> b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
> index 12d281e..cdbd7dc 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
> @@ -84,8 +84,8 @@ var_from_reg(const simple_allocator &alloc, const
> src_reg &reg,
>  {
>     assert(reg.file == VGRF && reg.nr < alloc.count &&
>            reg.reg_offset < alloc.sizes[reg.nr] && c < 4);
> -   return (4 * (alloc.offsets[reg.nr] + reg.reg_offset) +
> -           BRW_GET_SWZ(reg.swizzle, c));
> +   return (8 * (alloc.offsets[reg.nr] + reg.reg_offset) +
> +           BRW_GET_SWZ(reg.swizzle, c) * MAX2(1, type_sz(reg.type) /
> 4));
>  }
>  
>  inline unsigned
> @@ -94,7 +94,8 @@ var_from_reg(const simple_allocator &alloc, const
> dst_reg &reg,
>  {
>     assert(reg.file == VGRF && reg.nr < alloc.count &&
>            reg.reg_offset < alloc.sizes[reg.nr] && c < 4);
> -   return 4 * (alloc.offsets[reg.nr] + reg.reg_offset) + c;
> +   return 8 * (alloc.offsets[reg.nr] + reg.reg_offset) +
> +          c * MAX2(1, type_sz(reg.type) / 4);
>  }
>  
>  } /* namespace brw */
Iago Toral Quiroga <itoral@igalia.com> writes:

> From: "Juan A. Suarez Romero" <jasuarez@igalia.com>
>
> Our current data flow analysis does not take into account that channels
> on 64-bit operands are 64-bit. This is a problem when the same register
> is accessed using both 64-bit and 32-bit channels. This is very common
> in operations where we need to access 64-bit data in 32-bit chunks,
> such as the double packing and packing operations.
>
> This patch changes the analysis by checking the bits that each source
> or destination datatype needs. Actually, rather than bits, we use
> blocks of 32bits, which is the minimum channel size.
>
> Because a vgrf can contain a dvec4 (256 bits), we reserve 8
> 32-bit blocks to map the channels.
> ---
>  .../dri/i965/brw_vec4_dead_code_eliminate.cpp      | 30 +++++++++++++-----
>  .../drivers/dri/i965/brw_vec4_live_variables.cpp   | 36 +++++++++++++++++-----
>  .../drivers/dri/i965/brw_vec4_live_variables.h     |  7 +++--
>  3 files changed, 55 insertions(+), 18 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> index c643212..1f7cd49 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> @@ -59,10 +59,16 @@ vec4_visitor::dead_code_eliminate()
>              bool result_live[4] = { false };
>  
>              if (inst->dst.file == VGRF) {
> +               const unsigned offs = type_sz(inst->dst.type) == 8 ?
> +                  1 : (inst->exec_size == 4 ? 0 : 4);

I suggest that instead of duplicating this calculation in every single
user of the live sets you handle this in the var_from_reg helpers by
adding a new argument to specify which dword of the register component
you want to take, like:

| inline unsigned
| var_from_reg(const simple_allocator &alloc, const src_reg &reg,
|              unsigned c = 0, unsigned k = 0);
|
| inline unsigned
| var_from_reg(const simple_allocator &alloc, const dst_reg &reg,
|              unsigned c = 0, unsigned k = 0);

This would give you the variable index for the k-th dword of the c-th
component of register reg (feel free to write it in a comment for the
sake of documentation).  Doing it in a single place would be
advantageous because we are likely to have to change the calculation in
the future (e.g. to account for strides), and going through every user
will be a pain.  You could implement it as follows:

| inline unsigned
| var_from_reg(const simple_allocator &alloc, const src_reg &reg,
|              unsigned c = 0, unsigned i = 0)
| {
|    /* ... */
|    const unsigned csize = DIV_ROUND_UP(type_sz(reg.type), 4);
|    return 8 * (alloc.offsets[reg.nr] + reg.reg_offset) +
|           (BRW_GET_SWZ(reg.swizzle, c) + i / csize * 4) * csize +
|           i % csize;
| }

>                 for (unsigned i = 0; i < inst->regs_written; i++) {
> -                  for (int c = 0; c < 4; c++)
> -                     result_live[c] |= BITSET_TEST(
> -                        live, var_from_reg(alloc, offset(inst->dst, i), c));
> +                  for (int c = 0; c < 4; c++) {
> +                     const unsigned v =
> +                        var_from_reg(alloc, offset(inst->dst, i), c);
> +                     result_live[c] |= BITSET_TEST(live, v);
> +                     if (offs)
> +                        result_live[c] |= BITSET_TEST(live, v + offs);
> +                  }

Then you could simplify this and most of the following hunks massively,
like:

|               for (unsigned i = 0; i < 2 * inst->regs_written; i++) {
|                  for (int c = 0; c < 4; c++)
|                     result_live[c] |= BITSET_TEST(
|                        live, var_from_reg(alloc, inst->dst, c, i));
|               }

Note that the offset call goes away.  The factor of two makes sense
because you need to check 4 * 2 * regs_written bits in total, since
you're keeping track of 8 bits per GRF.  It would likely make sense to
represent vec4_instruction::regs_written and ::regs_read with smaller
granularity (e.g. in bytes) to avoid losing accuracy with sub-GRF writes
and reads, but that can probably be done in a separate change.

>                 }
>              } else {
>                 for (unsigned c = 0; c < 4; c++)
> @@ -110,11 +116,16 @@ vec4_visitor::dead_code_eliminate()
>           }
>  
>           if (inst->dst.file == VGRF && !inst->predicate) {
> +            const unsigned offs = type_sz(inst->dst.type) == 8 ?
> +               1 : (inst->exec_size == 4 ? 0 : 4);
>              for (unsigned i = 0; i < inst->regs_written; i++) {
>                 for (int c = 0; c < 4; c++) {
>                    if (inst->dst.writemask & (1 << c)) {
> -                     BITSET_CLEAR(live, var_from_reg(alloc,
> -                                                     offset(inst->dst, i), c));
> +                     const unsigned v =
> +                        var_from_reg(alloc, offset(inst->dst, i), c);
> +                     BITSET_CLEAR(live, v);
> +                     if (offs)
> +                        BITSET_CLEAR(live, v + offs);
>                    }
>                 }
>              }
> @@ -132,10 +143,15 @@ vec4_visitor::dead_code_eliminate()
>  
>           for (int i = 0; i < 3; i++) {
>              if (inst->src[i].file == VGRF) {
> +               const unsigned offs = type_sz(inst->src[i].type) == 8 ?
> +                  1 : (inst->exec_size == 4 ? 0 : 4);
>                 for (unsigned j = 0; j < inst->regs_read(i); j++) {
>                    for (int c = 0; c < 4; c++) {
> -                     BITSET_SET(live, var_from_reg(alloc,
> -                                                   offset(inst->src[i], j), c));
> +                     const unsigned v =
> +                        var_from_reg(alloc, offset(inst->src[i], j), c);
> +                     BITSET_SET(live, v);
> +                     if (offs)
> +                        BITSET_SET(live, v + offs);
>                    }
>                 }
>              }
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> index 57d5fbb..bfc5e44 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> @@ -76,12 +76,16 @@ vec4_live_variables::setup_def_use()
>  	 /* Set use[] for this instruction */
>  	 for (unsigned int i = 0; i < 3; i++) {
>  	    if (inst->src[i].file == VGRF) {
> +               const unsigned offs = type_sz(inst->dst.type) == 8 ?
> +                  1 : (inst->exec_size == 4 ? 0 : 4);
>                 for (unsigned j = 0; j < inst->regs_read(i); j++) {
>                    for (int c = 0; c < 4; c++) {
>                       const unsigned v =
>                          var_from_reg(alloc, offset(inst->src[i], j), c);
>                       if (!BITSET_TEST(bd->def, v))
>                          BITSET_SET(bd->use, v);
> +                     if (offs && !BITSET_TEST(bd->def, v + offs))
> +                        BITSET_SET(bd->use, v + offs);
>                    }
>                 }
>  	    }
> @@ -99,6 +103,8 @@ vec4_live_variables::setup_def_use()
>  	  */
>  	 if (inst->dst.file == VGRF &&
>  	     (!inst->predicate || inst->opcode == BRW_OPCODE_SEL)) {
> +            const unsigned offs = type_sz(inst->dst.type) == 8 ?
> +               1 : (inst->exec_size == 4 ? 0 : 4);
>              for (unsigned i = 0; i < inst->regs_written; i++) {
>                 for (int c = 0; c < 4; c++) {
>                    if (inst->dst.writemask & (1 << c)) {
> @@ -106,6 +112,8 @@ vec4_live_variables::setup_def_use()
>                          var_from_reg(alloc, offset(inst->dst, i), c);
>                       if (!BITSET_TEST(bd->use, v))
>                          BITSET_SET(bd->def, v);
> +                     if (offs && !BITSET_TEST(bd->use, v + offs))
> +                        BITSET_SET(bd->def, v + offs);
>                    }
>                 }
>              }
> @@ -188,7 +196,7 @@ vec4_live_variables::vec4_live_variables(const simple_allocator &alloc,
>  {
>     mem_ctx = ralloc_context(NULL);
>  
> -   num_vars = alloc.total_size * 4;
> +   num_vars = alloc.total_size * 8;
>     block_data = rzalloc_array(mem_ctx, struct block_data, cfg->num_blocks);
>  
>     bitset_words = BITSET_WORDS(num_vars);
> @@ -238,14 +246,14 @@ vec4_visitor::calculate_live_intervals()
>     if (this->live_intervals)
>        return;
>  
> -   int *start = ralloc_array(mem_ctx, int, this->alloc.total_size * 4);
> -   int *end = ralloc_array(mem_ctx, int, this->alloc.total_size * 4);
> +   int *start = ralloc_array(mem_ctx, int, this->alloc.total_size * 8);
> +   int *end = ralloc_array(mem_ctx, int, this->alloc.total_size * 8);
>     ralloc_free(this->virtual_grf_start);
>     ralloc_free(this->virtual_grf_end);
>     this->virtual_grf_start = start;
>     this->virtual_grf_end = end;
>  
> -   for (unsigned i = 0; i < this->alloc.total_size * 4; i++) {
> +   for (unsigned i = 0; i < this->alloc.total_size * 8; i++) {
>        start[i] = MAX_INSTRUCTION;
>        end[i] = -1;
>     }
> @@ -257,18 +265,26 @@ vec4_visitor::calculate_live_intervals()
>     foreach_block_and_inst(block, vec4_instruction, inst, cfg) {
>        for (unsigned int i = 0; i < 3; i++) {
>  	 if (inst->src[i].file == VGRF) {
> +            const unsigned offs = type_sz(inst->dst.type) == 8 ?
> +               1 : (inst->exec_size == 4 ? 0 : 4);
>              for (unsigned j = 0; j < inst->regs_read(i); j++) {
>                 for (int c = 0; c < 4; c++) {
>                    const unsigned v =
>                       var_from_reg(alloc, offset(inst->src[i], j), c);
>                    start[v] = MIN2(start[v], ip);
>                    end[v] = ip;
> +                  if (offs) {
> +                     start[v + offs] = MIN2(start[v + offs], ip);
> +                     end[v + offs] = ip;
> +                  }
>                 }
>              }
>  	 }
>        }
>  
>        if (inst->dst.file == VGRF) {
> +         const unsigned offs = type_sz(inst->dst.type) == 8 ?
> +            1 : (inst->exec_size == 4 ? 0 : 4);
>           for (unsigned i = 0; i < inst->regs_written; i++) {
>              for (int c = 0; c < 4; c++) {
>                 if (inst->dst.writemask & (1 << c)) {
> @@ -276,6 +292,10 @@ vec4_visitor::calculate_live_intervals()
>                       var_from_reg(alloc, offset(inst->dst, i), c);
>                    start[v] = MIN2(start[v], ip);
>                    end[v] = ip;
> +                  if (offs) {
> +                     start[v + offs] = MIN2(start[v + offs], ip);
> +                     end[v + offs] = ip;
> +                  }
>                 }
>              }
>           }
> @@ -340,8 +360,8 @@ vec4_visitor::var_range_end(unsigned v, unsigned n) const
>  bool
>  vec4_visitor::virtual_grf_interferes(int a, int b)
>  {
> -   return !((var_range_end(4 * alloc.offsets[a], 4 * alloc.sizes[a]) <=
> -             var_range_start(4 * alloc.offsets[b], 4 * alloc.sizes[b])) ||
> -            (var_range_end(4 * alloc.offsets[b], 4 * alloc.sizes[b]) <=
> -             var_range_start(4 * alloc.offsets[a], 4 * alloc.sizes[a])));
> +   return !((var_range_end(8 * alloc.offsets[a], 8 * alloc.sizes[a]) <=
> +             var_range_start(8 * alloc.offsets[b], 8 * alloc.sizes[b])) ||
> +            (var_range_end(8 * alloc.offsets[b], 8 * alloc.sizes[b]) <=
> +             var_range_start(8 * alloc.offsets[a], 8 * alloc.sizes[a])));
>  }
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
> index 12d281e..cdbd7dc 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
> @@ -84,8 +84,8 @@ var_from_reg(const simple_allocator &alloc, const src_reg &reg,
>  {
>     assert(reg.file == VGRF && reg.nr < alloc.count &&
>            reg.reg_offset < alloc.sizes[reg.nr] && c < 4);
> -   return (4 * (alloc.offsets[reg.nr] + reg.reg_offset) +
> -           BRW_GET_SWZ(reg.swizzle, c));
> +   return (8 * (alloc.offsets[reg.nr] + reg.reg_offset) +
> +           BRW_GET_SWZ(reg.swizzle, c) * MAX2(1, type_sz(reg.type) / 4));
>  }
>  
>  inline unsigned
> @@ -94,7 +94,8 @@ var_from_reg(const simple_allocator &alloc, const dst_reg &reg,
>  {
>     assert(reg.file == VGRF && reg.nr < alloc.count &&
>            reg.reg_offset < alloc.sizes[reg.nr] && c < 4);
> -   return 4 * (alloc.offsets[reg.nr] + reg.reg_offset) + c;
> +   return 8 * (alloc.offsets[reg.nr] + reg.reg_offset) +
> +          c * MAX2(1, type_sz(reg.type) / 4);
>  }
>  
>  } /* namespace brw */

I think there are a few cases you haven't updated for this change of
representation of live variables:

 - brw_vec4.cpp:1158 ("var_range_end(var_from_reg(alloc, inst->src[0]), 4) > ip")
 - brw_vec4_cse.cpp:267 ("var_range_end(var_from_reg(alloc, *src), 4) < ip")

BTW, have you tried running shader-db after and before this series, in
order to make sure that your optimizer changes didn't inadvertently
cause instruction or cycle count regressions?

> -- 
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Fri, 2016-07-29 at 12:59 -0700, Francisco Jerez wrote:
> Iago Toral Quiroga <itoral@igalia.com> writes:
> 
> > 
> > From: "Juan A. Suarez Romero" <jasuarez@igalia.com>
> > 
> > Our current data flow analysis does not take into account that
> > channels
> > on 64-bit operands are 64-bit. This is a problem when the same
> > register
> > is accessed using both 64-bit and 32-bit channels. This is very
> > common
> > in operations where we need to access 64-bit data in 32-bit chunks,
> > such as the double packing and packing operations.
> > 
> > This patch changes the analysis by checking the bits that each
> > source
> > or destination datatype needs. Actually, rather than bits, we use
> > blocks of 32bits, which is the minimum channel size.
> > 
> > Because a vgrf can contain a dvec4 (256 bits), we reserve 8
> > 32-bit blocks to map the channels.
> > ---
> >  .../dri/i965/brw_vec4_dead_code_eliminate.cpp      | 30
> > +++++++++++++-----
> >  .../drivers/dri/i965/brw_vec4_live_variables.cpp   | 36
> > +++++++++++++++++-----
> >  .../drivers/dri/i965/brw_vec4_live_variables.h     |  7 +++--
> >  3 files changed, 55 insertions(+), 18 deletions(-)
> > 
> > diff --git
> > a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> > b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> > index c643212..1f7cd49 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> > @@ -59,10 +59,16 @@ vec4_visitor::dead_code_eliminate()
> >              bool result_live[4] = { false };
> >  
> >              if (inst->dst.file == VGRF) {
> > +               const unsigned offs = type_sz(inst->dst.type) == 8
> > ?
> > +                  1 : (inst->exec_size == 4 ? 0 : 4);
> 
> I suggest that instead of duplicating this calculation in every
> single
> user of the live sets you handle this in the var_from_reg helpers by
> adding a new argument to specify which dword of the register
> component
> you want to take, like:
> 
> > 
> > inline unsigned
> > var_from_reg(const simple_allocator &alloc, const src_reg &reg,
> >              unsigned c = 0, unsigned k = 0);
> > 
> > inline unsigned
> > var_from_reg(const simple_allocator &alloc, const dst_reg &reg,
> >              unsigned c = 0, unsigned k = 0);
> 
> This would give you the variable index for the k-th dword of the c-th
> component of register reg (feel free to write it in a comment for the
> sake of documentation).  Doing it in a single place would be
> advantageous because we are likely to have to change the calculation
> in
> the future (e.g. to account for strides), and going through every
> user
> will be a pain.  You could implement it as follows:
> 
> > 
> > inline unsigned
> > var_from_reg(const simple_allocator &alloc, const src_reg &reg,
> >              unsigned c = 0, unsigned i = 0)
> > {
> >    /* ... */
> >    const unsigned csize = DIV_ROUND_UP(type_sz(reg.type), 4);
> >    return 8 * (alloc.offsets[reg.nr] + reg.reg_offset) +
> >           (BRW_GET_SWZ(reg.swizzle, c) + i / csize * 4) * csize +
> >           i % csize;
> > }
> 
> > 
> >                 for (unsigned i = 0; i < inst->regs_written; i++) {
> > -                  for (int c = 0; c < 4; c++)
> > -                     result_live[c] |= BITSET_TEST(
> > -                        live, var_from_reg(alloc, offset(inst-
> > >dst, i), c));
> > +                  for (int c = 0; c < 4; c++) {
> > +                     const unsigned v =
> > +                        var_from_reg(alloc, offset(inst->dst, i),
> > c);
> > +                     result_live[c] |= BITSET_TEST(live, v);
> > +                     if (offs)
> > +                        result_live[c] |= BITSET_TEST(live, v +
> > offs);
> > +                  }
> 
> Then you could simplify this and most of the following hunks
> massively,
> like:
> 
> > 
> >               for (unsigned i = 0; i < 2 * inst->regs_written; i++)
> > {
> >                  for (int c = 0; c < 4; c++)
> >                     result_live[c] |= BITSET_TEST(
> >                        live, var_from_reg(alloc, inst->dst, c, i));
> >               }
> 
> Note that the offset call goes away.  The factor of two makes sense
> because you need to check 4 * 2 * regs_written bits in total, since
> you're keeping track of 8 bits per GRF.  It would likely make sense
> to
> represent vec4_instruction::regs_written and ::regs_read with smaller
> granularity (e.g. in bytes) to avoid losing accuracy with sub-GRF
> writes
> and reads, but that can probably be done in a separate change.

Thanks for the suggestion!! I'll proceed as you said.


> 
> > 
> >                 }
> >              } else {
> >                 for (unsigned c = 0; c < 4; c++)
> > @@ -110,11 +116,16 @@ vec4_visitor::dead_code_eliminate()
> >           }
> >  
> >           if (inst->dst.file == VGRF && !inst->predicate) {
> > +            const unsigned offs = type_sz(inst->dst.type) == 8 ?
> > +               1 : (inst->exec_size == 4 ? 0 : 4);
> >              for (unsigned i = 0; i < inst->regs_written; i++) {
> >                 for (int c = 0; c < 4; c++) {
> >                    if (inst->dst.writemask & (1 << c)) {
> > -                     BITSET_CLEAR(live, var_from_reg(alloc,
> > -                                                     offset(inst-
> > >dst, i), c));
> > +                     const unsigned v =
> > +                        var_from_reg(alloc, offset(inst->dst, i),
> > c);
> > +                     BITSET_CLEAR(live, v);
> > +                     if (offs)
> > +                        BITSET_CLEAR(live, v + offs);
> >                    }
> >                 }
> >              }
> > @@ -132,10 +143,15 @@ vec4_visitor::dead_code_eliminate()
> >  
> >           for (int i = 0; i < 3; i++) {
> >              if (inst->src[i].file == VGRF) {
> > +               const unsigned offs = type_sz(inst->src[i].type) ==
> > 8 ?
> > +                  1 : (inst->exec_size == 4 ? 0 : 4);
> >                 for (unsigned j = 0; j < inst->regs_read(i); j++) {
> >                    for (int c = 0; c < 4; c++) {
> > -                     BITSET_SET(live, var_from_reg(alloc,
> > -                                                   offset(inst-
> > >src[i], j), c));
> > +                     const unsigned v =
> > +                        var_from_reg(alloc, offset(inst->src[i],
> > j), c);
> > +                     BITSET_SET(live, v);
> > +                     if (offs)
> > +                        BITSET_SET(live, v + offs);
> >                    }
> >                 }
> >              }
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> > b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> > index 57d5fbb..bfc5e44 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> > @@ -76,12 +76,16 @@ vec4_live_variables::setup_def_use()
> >  	 /* Set use[] for this instruction */
> >  	 for (unsigned int i = 0; i < 3; i++) {
> >  	    if (inst->src[i].file == VGRF) {
> > +               const unsigned offs = type_sz(inst->dst.type) == 8
> > ?
> > +                  1 : (inst->exec_size == 4 ? 0 : 4);
> >                 for (unsigned j = 0; j < inst->regs_read(i); j++) {
> >                    for (int c = 0; c < 4; c++) {
> >                       const unsigned v =
> >                          var_from_reg(alloc, offset(inst->src[i],
> > j), c);
> >                       if (!BITSET_TEST(bd->def, v))
> >                          BITSET_SET(bd->use, v);
> > +                     if (offs && !BITSET_TEST(bd->def, v + offs))
> > +                        BITSET_SET(bd->use, v + offs);
> >                    }
> >                 }
> >  	    }
> > @@ -99,6 +103,8 @@ vec4_live_variables::setup_def_use()
> >  	  */
> >  	 if (inst->dst.file == VGRF &&
> >  	     (!inst->predicate || inst->opcode == BRW_OPCODE_SEL))
> > {
> > +            const unsigned offs = type_sz(inst->dst.type) == 8 ?
> > +               1 : (inst->exec_size == 4 ? 0 : 4);
> >              for (unsigned i = 0; i < inst->regs_written; i++) {
> >                 for (int c = 0; c < 4; c++) {
> >                    if (inst->dst.writemask & (1 << c)) {
> > @@ -106,6 +112,8 @@ vec4_live_variables::setup_def_use()
> >                          var_from_reg(alloc, offset(inst->dst, i),
> > c);
> >                       if (!BITSET_TEST(bd->use, v))
> >                          BITSET_SET(bd->def, v);
> > +                     if (offs && !BITSET_TEST(bd->use, v + offs))
> > +                        BITSET_SET(bd->def, v + offs);
> >                    }
> >                 }
> >              }
> > @@ -188,7 +196,7 @@ vec4_live_variables::vec4_live_variables(const
> > simple_allocator &alloc,
> >  {
> >     mem_ctx = ralloc_context(NULL);
> >  
> > -   num_vars = alloc.total_size * 4;
> > +   num_vars = alloc.total_size * 8;
> >     block_data = rzalloc_array(mem_ctx, struct block_data, cfg-
> > >num_blocks);
> >  
> >     bitset_words = BITSET_WORDS(num_vars);
> > @@ -238,14 +246,14 @@ vec4_visitor::calculate_live_intervals()
> >     if (this->live_intervals)
> >        return;
> >  
> > -   int *start = ralloc_array(mem_ctx, int, this->alloc.total_size
> > * 4);
> > -   int *end = ralloc_array(mem_ctx, int, this->alloc.total_size *
> > 4);
> > +   int *start = ralloc_array(mem_ctx, int, this->alloc.total_size
> > * 8);
> > +   int *end = ralloc_array(mem_ctx, int, this->alloc.total_size *
> > 8);
> >     ralloc_free(this->virtual_grf_start);
> >     ralloc_free(this->virtual_grf_end);
> >     this->virtual_grf_start = start;
> >     this->virtual_grf_end = end;
> >  
> > -   for (unsigned i = 0; i < this->alloc.total_size * 4; i++) {
> > +   for (unsigned i = 0; i < this->alloc.total_size * 8; i++) {
> >        start[i] = MAX_INSTRUCTION;
> >        end[i] = -1;
> >     }
> > @@ -257,18 +265,26 @@ vec4_visitor::calculate_live_intervals()
> >     foreach_block_and_inst(block, vec4_instruction, inst, cfg) {
> >        for (unsigned int i = 0; i < 3; i++) {
> >  	 if (inst->src[i].file == VGRF) {
> > +            const unsigned offs = type_sz(inst->dst.type) == 8 ?
> > +               1 : (inst->exec_size == 4 ? 0 : 4);
> >              for (unsigned j = 0; j < inst->regs_read(i); j++) {
> >                 for (int c = 0; c < 4; c++) {
> >                    const unsigned v =
> >                       var_from_reg(alloc, offset(inst->src[i], j),
> > c);
> >                    start[v] = MIN2(start[v], ip);
> >                    end[v] = ip;
> > +                  if (offs) {
> > +                     start[v + offs] = MIN2(start[v + offs], ip);
> > +                     end[v + offs] = ip;
> > +                  }
> >                 }
> >              }
> >  	 }
> >        }
> >  
> >        if (inst->dst.file == VGRF) {
> > +         const unsigned offs = type_sz(inst->dst.type) == 8 ?
> > +            1 : (inst->exec_size == 4 ? 0 : 4);
> >           for (unsigned i = 0; i < inst->regs_written; i++) {
> >              for (int c = 0; c < 4; c++) {
> >                 if (inst->dst.writemask & (1 << c)) {
> > @@ -276,6 +292,10 @@ vec4_visitor::calculate_live_intervals()
> >                       var_from_reg(alloc, offset(inst->dst, i), c);
> >                    start[v] = MIN2(start[v], ip);
> >                    end[v] = ip;
> > +                  if (offs) {
> > +                     start[v + offs] = MIN2(start[v + offs], ip);
> > +                     end[v + offs] = ip;
> > +                  }
> >                 }
> >              }
> >           }
> > @@ -340,8 +360,8 @@ vec4_visitor::var_range_end(unsigned v,
> > unsigned n) const
> >  bool
> >  vec4_visitor::virtual_grf_interferes(int a, int b)
> >  {
> > -   return !((var_range_end(4 * alloc.offsets[a], 4 *
> > alloc.sizes[a]) <=
> > -             var_range_start(4 * alloc.offsets[b], 4 *
> > alloc.sizes[b])) ||
> > -            (var_range_end(4 * alloc.offsets[b], 4 *
> > alloc.sizes[b]) <=
> > -             var_range_start(4 * alloc.offsets[a], 4 *
> > alloc.sizes[a])));
> > +   return !((var_range_end(8 * alloc.offsets[a], 8 *
> > alloc.sizes[a]) <=
> > +             var_range_start(8 * alloc.offsets[b], 8 *
> > alloc.sizes[b])) ||
> > +            (var_range_end(8 * alloc.offsets[b], 8 *
> > alloc.sizes[b]) <=
> > +             var_range_start(8 * alloc.offsets[a], 8 *
> > alloc.sizes[a])));
> >  }
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
> > b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
> > index 12d281e..cdbd7dc 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.h
> > @@ -84,8 +84,8 @@ var_from_reg(const simple_allocator &alloc, const
> > src_reg &reg,
> >  {
> >     assert(reg.file == VGRF && reg.nr < alloc.count &&
> >            reg.reg_offset < alloc.sizes[reg.nr] && c < 4);
> > -   return (4 * (alloc.offsets[reg.nr] + reg.reg_offset) +
> > -           BRW_GET_SWZ(reg.swizzle, c));
> > +   return (8 * (alloc.offsets[reg.nr] + reg.reg_offset) +
> > +           BRW_GET_SWZ(reg.swizzle, c) * MAX2(1, type_sz(reg.type)
> > / 4));
> >  }
> >  
> >  inline unsigned
> > @@ -94,7 +94,8 @@ var_from_reg(const simple_allocator &alloc, const
> > dst_reg &reg,
> >  {
> >     assert(reg.file == VGRF && reg.nr < alloc.count &&
> >            reg.reg_offset < alloc.sizes[reg.nr] && c < 4);
> > -   return 4 * (alloc.offsets[reg.nr] + reg.reg_offset) + c;
> > +   return 8 * (alloc.offsets[reg.nr] + reg.reg_offset) +
> > +          c * MAX2(1, type_sz(reg.type) / 4);
> >  }
> >  
> >  } /* namespace brw */
> 
> I think there are a few cases you haven't updated for this change of
> representation of live variables:
> 
>  - brw_vec4.cpp:1158 ("var_range_end(var_from_reg(alloc, inst-
> >src[0]), 4) > ip")
>  - brw_vec4_cse.cpp:267 ("var_range_end(var_from_reg(alloc, *src), 4)
> < ip")
> 
> BTW, have you tried running shader-db after and before this series,
> in
> order to make sure that your optimizer changes didn't inadvertently
> cause instruction or cycle count regressions?
> 


No, I did not. I will do. Thank you for the notice.


	J.A.
On Fri, 2016-07-29 at 12:59 -0700, Francisco Jerez wrote:
> |               for (unsigned i = 0; i < 2 * inst->regs_written; i++)
> {
> |                  for (int c = 0; c < 4; c++)
> |                     result_live[c] |= BITSET_TEST(
> |                        live, var_from_reg(alloc, inst->dst, c, i));
> |               }
> 
> Note that the offset call goes away.  The factor of two makes sense
> because you need to check 4 * 2 * regs_written bits in total, since
> you're keeping track of 8 bits per GRF.  It would likely make sense
> to

Hmm... what about the case of exec_size == 4 and writing just a float? 

I understand in this case we only should mark one word, so the loop
should  not be 2*inst->regs_written.

Note that in the original patch that was the case for using offs == 0.
So I guess we should compute if we should use 1 or 2 times
regs_written.


	J.A.
On Mon, 2016-08-08 at 16:12 +0200, Juan A. Suarez Romero wrote:
> Hmm... what about the case of exec_size == 4 and writing just a
> float? 
> 
> I understand in this case we only should mark one word, so the loop
> should  not be 2*inst->regs_written.
> 
> Note that in the original patch that was the case for using offs ==
> 0.
> So I guess we should compute if we should use 1 or 2 times
> regs_written.


Thinking more carefully, I think it is fine using 2*regs_written/read
in this case.

So forget about my comment!


	J.A.
"Juan A. Suarez Romero" <jasuarez@igalia.com> writes:

> On Mon, 2016-08-08 at 16:12 +0200, Juan A. Suarez Romero wrote:
>> Hmm... what about the case of exec_size == 4 and writing just a
>> float? 
>> 
>> I understand in this case we only should mark one word, so the loop
>> should  not be 2*inst->regs_written.
>> 
>> Note that in the original patch that was the case for using offs ==
>> 0.
>> So I guess we should compute if we should use 1 or 2 times
>> regs_written.
>
>
> Thinking more carefully, I think it is fine using 2*regs_written/read
> in this case.
>

The fundamental problem is that regs_written is expressed in units of
whole GRFs, but SIMD4 single-precision instructions typically write less
than one GRF, which will cause you to overestimate the number of dwords
written or read by the instruction (that's exactly what I was referring
to earlier with losing accuracy with sub-GRF writes).

I wouldn't worry too much about this right now, because it will only
cause you to calculate slightly pessimistic live intervals for the
variables used in such instructions, and shouldn't ever lead to program
miscompilation.  Overriding the dwords-per-vec4-component factor
depending on the SIMD width of the instruction seems like a hack that
could potentially hurt more than it would help (What if a
single-precision SIMD4 instruction actually reads or writes the whole
GRF register because it has multiple output phases?  How do you know?).

The right solution would be to represent regs_written in units that give
you sufficient accuracy, but I think there is enough going on in this
series that I wouldn't bother to fix that in addition.  We hadn't been
using SIMD4 instructions in the vec4 back-end until now, which means
that this inaccuracy shouldn't affect existing code or lead to
performance regressions, and the FS back-end has exactly the same
problem so you could potentially fix both at the same time. ;)

> So forget about my comment!
>
>
> 	J.A.