Re: [Mesa-dev] [PATCH v3] nir: propagates if condition evaluation down some alu chains

2018-08-29 Thread Timothy Arceri 

On 30/08/18 11:05, Ian Romanick wrote:

I feel like this would be a good candidate for some unit tests.

As a follow up, you might also consider adding bcsel.  I have seen quite
a few cases of bcsel(bool, bool, bool).


Ah yes that is probably a good one to try. I'll give it a go.



On 08/29/2018 04:48 PM, Timothy Arceri wrote:

v2:
  - only allow nir_op_inot or nir_op_b2i when alu input is 1.
  - use some helpers as suggested by Jason.

v3:
  - evaluate alu op for single input alu ops
  - add helper function to decide if to propagate through alu
  - make use of nir_before_src in another spot

shader-db IVB results:

total instructions in shared programs: 9993483 -> 9993472 (-0.00%)
instructions in affected programs: 1300 -> 1289 (-0.85%)
helped: 11
HURT: 0

total cycles in shared programs: 219476091 -> 219476059 (-0.00%)
cycles in affected programs: 7675 -> 7643 (-0.42%)
helped: 10
HURT: 1
---
  src/compiler/nir/nir_opt_if.c | 145 --
  1 file changed, 139 insertions(+), 6 deletions(-)

diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
index 11c6693d302..9e9d8edda21 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -23,6 +23,7 @@
  
  #include "nir.h"

  #include "nir/nir_builder.h"
+#include "nir_constant_expressions.h"
  #include "nir_control_flow.h"
  #include "nir_loop_analyze.h"
  
@@ -403,9 +404,127 @@ evaluate_if_condition(nir_if *nif, nir_cursor cursor, uint32_t *value)

 }
  }
  
+/*

+ * This propagates if condition evaluation down the chain of some alu
+ * instructions. For example by checking the use of some of the following alu
+ * instruction we can eventually replace ssa_107 with NIR_TRUE.
+ *
+ *   loop {
+ *  block block_1:
+ *  vec1 32 ssa_85 = load_const (0x0002)
+ *  vec1 32 ssa_86 = ieq ssa_48, ssa_85
+ *  vec1 32 ssa_87 = load_const (0x0001)
+ *  vec1 32 ssa_88 = ieq ssa_48, ssa_87
+ *  vec1 32 ssa_89 = ior ssa_86, ssa_88
+ *  vec1 32 ssa_90 = ieq ssa_48, ssa_0
+ *  vec1 32 ssa_91 = ior ssa_89, ssa_90
+ *  if ssa_86 {
+ * block block_2:
+ * ...
+ *break
+ *  } else {
+ *block block_3:
+ *  }
+ *  block block_4:
+ *  if ssa_88 {
+ *block block_5:
+ * ...
+ *break
+ *  } else {
+ *block block_6:
+ *  }
+ *  block block_7:
+ *  if ssa_90 {
+ *block block_8:
+ * ...
+ *break
+ *  } else {
+ *block block_9:
+ *  }
+ *  block block_10:
+ *  vec1 32 ssa_107 = inot ssa_91
+ *  if ssa_107 {
+ *block block_11:
+ *break
+ *  } else {
+ *block block_12:
+ *  }
+ *   }
+ */
  static bool
-evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,
-   bool is_if_condition)
+propagate_condition_eval(nir_builder *b, nir_if *nif, nir_src *use_src,
+ nir_src *alu_use, nir_alu_instr *alu, void *mem_ctx,
+ bool is_if_condition)
+{
+   bool progress = false;
+
+   uint32_t bool_value;
+   b->cursor = nir_before_src(alu_use, is_if_condition);
+   if (nir_op_infos[alu->op].num_inputs == 1) {
+  assert(alu->op == nir_op_inot || alu->op == nir_op_b2i);
+
+  if (evaluate_if_condition(nif, b->cursor, _value)) {
+ nir_const_value bool_src;
+ bool_src.u32[0] = bool_value;
+
+ unsigned bit_size = nir_src_bit_size(alu->src[0].src);
+ nir_const_value result =
+nir_eval_const_opcode(alu->op, 1, bit_size, _src);
+
+ replace_if_condition_use_with_const(alu_use, mem_ctx, b->cursor,
+ result.u32[0], is_if_condition);
+ progress = true;
+  }
+   } else {
+  assert(alu->op == nir_op_ior || alu->op == nir_op_iand);
+
+  if (evaluate_if_condition(nif, b->cursor, _value)) {
+ nir_ssa_def *def[2];
+ for (unsigned i = 0; i < 2; i++) {
+if (alu->src[i].src.ssa == use_src->ssa) {
+   nir_const_value const_value;
+   const_value.u32[0] = bool_value;
+
+   def[i] = nir_build_imm(b, 1, 32, const_value);
+} else {
+   def[i] = alu->src[i].src.ssa;
+}
+ }
+
+ nir_ssa_def *nalu =
+nir_build_alu(b, alu->op, def[0], def[1], NULL, NULL);
+
+ /* Rewrite use to use new alu instruction */
+ nir_src new_src = nir_src_for_ssa(nalu);
+
+ if (is_if_condition)
+nir_if_rewrite_condition(alu_use->parent_if, new_src);
+ else
+nir_instr_rewrite_src(alu_use->parent_instr, alu_use, new_src);
+
+ progress = true;
+  }
+   }
+
+   return progress;
+}
+
+static bool
+can_propagate_through_alu(nir_src *src)
+{
+   if (src->parent_instr->type == nir_instr_type_alu &&
+

Re: [Mesa-dev] [PATCH v3] nir: propagates if condition evaluation down some alu chains

2018-08-29 Thread Jason Ekstrand 
On Wed, Aug 29, 2018 at 6:48 PM Timothy Arceri 
wrote:

> v2:
>  - only allow nir_op_inot or nir_op_b2i when alu input is 1.
>  - use some helpers as suggested by Jason.
>
> v3:
>  - evaluate alu op for single input alu ops
>  - add helper function to decide if to propagate through alu
>  - make use of nir_before_src in another spot
>
> shader-db IVB results:
>
> total instructions in shared programs: 9993483 -> 9993472 (-0.00%)
> instructions in affected programs: 1300 -> 1289 (-0.85%)
> helped: 11
> HURT: 0
>
> total cycles in shared programs: 219476091 -> 219476059 (-0.00%)
> cycles in affected programs: 7675 -> 7643 (-0.42%)
> helped: 10
> HURT: 1
> ---
>  src/compiler/nir/nir_opt_if.c | 145 --
>  1 file changed, 139 insertions(+), 6 deletions(-)
>
> diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
> index 11c6693d302..9e9d8edda21 100644
> --- a/src/compiler/nir/nir_opt_if.c
> +++ b/src/compiler/nir/nir_opt_if.c
> @@ -23,6 +23,7 @@
>
>  #include "nir.h"
>  #include "nir/nir_builder.h"
> +#include "nir_constant_expressions.h"
>  #include "nir_control_flow.h"
>  #include "nir_loop_analyze.h"
>
> @@ -403,9 +404,127 @@ evaluate_if_condition(nir_if *nif, nir_cursor
> cursor, uint32_t *value)
> }
>  }
>
> +/*
> + * This propagates if condition evaluation down the chain of some alu
> + * instructions. For example by checking the use of some of the following
> alu
> + * instruction we can eventually replace ssa_107 with NIR_TRUE.
> + *
> + *   loop {
> + *  block block_1:
> + *  vec1 32 ssa_85 = load_const (0x0002)
> + *  vec1 32 ssa_86 = ieq ssa_48, ssa_85
> + *  vec1 32 ssa_87 = load_const (0x0001)
> + *  vec1 32 ssa_88 = ieq ssa_48, ssa_87
> + *  vec1 32 ssa_89 = ior ssa_86, ssa_88
> + *  vec1 32 ssa_90 = ieq ssa_48, ssa_0
> + *  vec1 32 ssa_91 = ior ssa_89, ssa_90
> + *  if ssa_86 {
> + * block block_2:
> + * ...
> + *break
> + *  } else {
> + *block block_3:
> + *  }
> + *  block block_4:
> + *  if ssa_88 {
> + *block block_5:
> + * ...
> + *break
> + *  } else {
> + *block block_6:
> + *  }
> + *  block block_7:
> + *  if ssa_90 {
> + *block block_8:
> + * ...
> + *break
> + *  } else {
> + *block block_9:
> + *  }
> + *  block block_10:
> + *  vec1 32 ssa_107 = inot ssa_91
> + *  if ssa_107 {
> + *block block_11:
> + *break
> + *  } else {
> + *block block_12:
> + *  }
> + *   }
> + */
>  static bool
> -evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,
> -   bool is_if_condition)
> +propagate_condition_eval(nir_builder *b, nir_if *nif, nir_src *use_src,
> + nir_src *alu_use, nir_alu_instr *alu, void
> *mem_ctx,
> + bool is_if_condition)
> +{
> +   bool progress = false;
> +
> +   uint32_t bool_value;
> +   b->cursor = nir_before_src(alu_use, is_if_condition);
> +   if (nir_op_infos[alu->op].num_inputs == 1) {
> +  assert(alu->op == nir_op_inot || alu->op == nir_op_b2i);
> +
> +  if (evaluate_if_condition(nif, b->cursor, _value)) {
> + nir_const_value bool_src;
> + bool_src.u32[0] = bool_value;
>

Since you're just going to put bool_value in bool_src and const_value
below, why not just have bool_value be a nir_const_value and do
evaluate_if_condition(nif, b->cursor, _value.u32[0]) and be done with
it?


> +
> + unsigned bit_size = nir_src_bit_size(alu->src[0].src);
>

This had better be 32 or we're toast because we're assuming a uint32_t the
whole time.  Maybe make this an assert instead?

+ nir_const_value result =
> +nir_eval_const_opcode(alu->op, 1, bit_size, _src);
> +
> + replace_if_condition_use_with_const(alu_use, mem_ctx, b->cursor,
> + result.u32[0],
> is_if_condition);
> + progress = true;
> +  }
> +   } else {
> +  assert(alu->op == nir_op_ior || alu->op == nir_op_iand);
> +
> +  if (evaluate_if_condition(nif, b->cursor, _value)) {
> + nir_ssa_def *def[2];
> + for (unsigned i = 0; i < 2; i++) {
> +if (alu->src[i].src.ssa == use_src->ssa) {
> +   nir_const_value const_value;
> +   const_value.u32[0] = bool_value;
> +
> +   def[i] = nir_build_imm(b, 1, 32, const_value);
>

We assume 32 here, for instance.

With the above two clean-ups,

Reviewed-by: Jason Ekstrand 


> +} else {
> +   def[i] = alu->src[i].src.ssa;
> +}
> + }
> +
> + nir_ssa_def *nalu =
> +nir_build_alu(b, alu->op, def[0], def[1], NULL, NULL);
> +
> + /* Rewrite use to use new alu instruction */
> + nir_src new_src = nir_src_for_ssa(nalu);
> +
>

Re: [Mesa-dev] [PATCH v3] nir: propagates if condition evaluation down some alu chains

2018-08-29 Thread Ian Romanick 
I feel like this would be a good candidate for some unit tests.

As a follow up, you might also consider adding bcsel.  I have seen quite
a few cases of bcsel(bool, bool, bool).

On 08/29/2018 04:48 PM, Timothy Arceri wrote:
> v2:
>  - only allow nir_op_inot or nir_op_b2i when alu input is 1.
>  - use some helpers as suggested by Jason.
> 
> v3:
>  - evaluate alu op for single input alu ops
>  - add helper function to decide if to propagate through alu
>  - make use of nir_before_src in another spot
> 
> shader-db IVB results:
> 
> total instructions in shared programs: 9993483 -> 9993472 (-0.00%)
> instructions in affected programs: 1300 -> 1289 (-0.85%)
> helped: 11
> HURT: 0
> 
> total cycles in shared programs: 219476091 -> 219476059 (-0.00%)
> cycles in affected programs: 7675 -> 7643 (-0.42%)
> helped: 10
> HURT: 1
> ---
>  src/compiler/nir/nir_opt_if.c | 145 --
>  1 file changed, 139 insertions(+), 6 deletions(-)
> 
> diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
> index 11c6693d302..9e9d8edda21 100644
> --- a/src/compiler/nir/nir_opt_if.c
> +++ b/src/compiler/nir/nir_opt_if.c
> @@ -23,6 +23,7 @@
>  
>  #include "nir.h"
>  #include "nir/nir_builder.h"
> +#include "nir_constant_expressions.h"
>  #include "nir_control_flow.h"
>  #include "nir_loop_analyze.h"
>  
> @@ -403,9 +404,127 @@ evaluate_if_condition(nir_if *nif, nir_cursor cursor, 
> uint32_t *value)
> }
>  }
>  
> +/*
> + * This propagates if condition evaluation down the chain of some alu
> + * instructions. For example by checking the use of some of the following alu
> + * instruction we can eventually replace ssa_107 with NIR_TRUE.
> + *
> + *   loop {
> + *  block block_1:
> + *  vec1 32 ssa_85 = load_const (0x0002)
> + *  vec1 32 ssa_86 = ieq ssa_48, ssa_85
> + *  vec1 32 ssa_87 = load_const (0x0001)
> + *  vec1 32 ssa_88 = ieq ssa_48, ssa_87
> + *  vec1 32 ssa_89 = ior ssa_86, ssa_88
> + *  vec1 32 ssa_90 = ieq ssa_48, ssa_0
> + *  vec1 32 ssa_91 = ior ssa_89, ssa_90
> + *  if ssa_86 {
> + * block block_2:
> + * ...
> + *break
> + *  } else {
> + *block block_3:
> + *  }
> + *  block block_4:
> + *  if ssa_88 {
> + *block block_5:
> + * ...
> + *break
> + *  } else {
> + *block block_6:
> + *  }
> + *  block block_7:
> + *  if ssa_90 {
> + *block block_8:
> + * ...
> + *break
> + *  } else {
> + *block block_9:
> + *  }
> + *  block block_10:
> + *  vec1 32 ssa_107 = inot ssa_91
> + *  if ssa_107 {
> + *block block_11:
> + *break
> + *  } else {
> + *block block_12:
> + *  }
> + *   }
> + */
>  static bool
> -evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,
> -   bool is_if_condition)
> +propagate_condition_eval(nir_builder *b, nir_if *nif, nir_src *use_src,
> + nir_src *alu_use, nir_alu_instr *alu, void *mem_ctx,
> + bool is_if_condition)
> +{
> +   bool progress = false;
> +
> +   uint32_t bool_value;
> +   b->cursor = nir_before_src(alu_use, is_if_condition);
> +   if (nir_op_infos[alu->op].num_inputs == 1) {
> +  assert(alu->op == nir_op_inot || alu->op == nir_op_b2i);
> +
> +  if (evaluate_if_condition(nif, b->cursor, _value)) {
> + nir_const_value bool_src;
> + bool_src.u32[0] = bool_value;
> +
> + unsigned bit_size = nir_src_bit_size(alu->src[0].src);
> + nir_const_value result =
> +nir_eval_const_opcode(alu->op, 1, bit_size, _src);
> +
> + replace_if_condition_use_with_const(alu_use, mem_ctx, b->cursor,
> + result.u32[0], is_if_condition);
> + progress = true;
> +  }
> +   } else {
> +  assert(alu->op == nir_op_ior || alu->op == nir_op_iand);
> +
> +  if (evaluate_if_condition(nif, b->cursor, _value)) {
> + nir_ssa_def *def[2];
> + for (unsigned i = 0; i < 2; i++) {
> +if (alu->src[i].src.ssa == use_src->ssa) {
> +   nir_const_value const_value;
> +   const_value.u32[0] = bool_value;
> +
> +   def[i] = nir_build_imm(b, 1, 32, const_value);
> +} else {
> +   def[i] = alu->src[i].src.ssa;
> +}
> + }
> +
> + nir_ssa_def *nalu =
> +nir_build_alu(b, alu->op, def[0], def[1], NULL, NULL);
> +
> + /* Rewrite use to use new alu instruction */
> + nir_src new_src = nir_src_for_ssa(nalu);
> +
> + if (is_if_condition)
> +nir_if_rewrite_condition(alu_use->parent_if, new_src);
> + else
> +nir_instr_rewrite_src(alu_use->parent_instr, alu_use, new_src);
> +
> + progress = true;
> +  }
> +   }
> +
> +