subject:"\[Mesa\-dev\] \[PATCH v4 1\/7\] nir\: evaluate if condition uses inside the if branches"

Re: [Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

2018-08-29 Thread Timothy Arceri


On 30/08/18 12:56, Jason Ekstrand wrote:
On Wed, Aug 29, 2018 at 9:45 PM Timothy Arceri > wrote:


On 30/08/18 10:57, Ian Romanick wrote:
 > On 08/27/2018 02:08 AM, Timothy Arceri wrote:
 >> Since we know what side of the branch we ended up on we can just
 >> replace the use with a constant.
 >>
 >> All the spill changes in shader-db are from Dolphin uber shaders,
 >> despite some small regressions the change is clearly positive.
 >>
 >> V2: insert new constant after any phis in the
 >>      use->parent_instr->type == nir_instr_type_phi path.
 >>
 >> v3:
 >>   - use nir_after_block_before_jump() for inserting const
 >>   - check dominance of phi uses correctly
 >>
 >> v4:
 >>   - create some helpers as suggested by Jason.
 >>
 >> shader-db results IVB:
 >>
 >> total instructions in shared programs: 201 -> 9993483 (-0.06%)
 >> instructions in affected programs: 163235 -> 157517 (-3.50%)
 >> helped: 132
 >> HURT: 2
 >>
 >> total cycles in shared programs: 231670754 -> 219476091 (-5.26%)
 >> cycles in affected programs: 143424120 -> 131229457 (-8.50%)
 >> helped: 115
 >> HURT: 24
 >>
 >> total spills in shared programs: 4383 -> 4370 (-0.30%)
 >> spills in affected programs: 1656 -> 1643 (-0.79%)
 >> helped: 9
 >> HURT: 18
 >>
 >> total fills in shared programs: 4610 -> 4581 (-0.63%)
 >> fills in affected programs: 374 -> 345 (-7.75%)
 >> helped: 6
 >> HURT: 0
 >> ---
 >>   src/compiler/nir/nir.h        |  22 +++
 >>   src/compiler/nir/nir_opt_if.c | 113
++
 >>   2 files changed, 135 insertions(+)
 >>
 >> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
 >> index 009a6d60371..0caacd30173 100644
 >> --- a/src/compiler/nir/nir.h
 >> +++ b/src/compiler/nir/nir.h
 >> @@ -2331,6 +2331,28 @@ nir_after_block_before_jump(nir_block *block)
 >>      }
 >>   }
 >>
 >> +static inline nir_cursor
 >> +nir_before_src(nir_src *src, bool is_if_condition)
 >> +{
 >> +   if (is_if_condition) {
 >> +      nir_block *prev_block =
 >> +   
  nir_cf_node_as_block(nir_cf_node_prev(>parent_if->cf_node));

 >> +      assert(!nir_block_ends_in_jump(prev_block));
 >> +      return nir_after_block(prev_block);
 >> +   } else if (src->parent_instr->type == nir_instr_type_phi) {
 >> +      nir_phi_instr *cond_phi =
nir_instr_as_phi(src->parent_instr);
 >> +      nir_foreach_phi_src(phi_src, cond_phi) {
 >> +         if (phi_src->src.ssa == src->ssa) {
 >> +            return nir_after_block_before_jump(phi_src->pred);
 >> +         }
 >> +      }
 >> +
 >> +      unreachable("failed to find phi src");
 >> +   } else {
 >> +      return nir_before_instr(src->parent_instr);
 >> +   }
 >> +}
 >> +
 >>   static inline nir_cursor
 >>   nir_before_cf_node(nir_cf_node *node)
 >>   {
 >> diff --git a/src/compiler/nir/nir_opt_if.c
b/src/compiler/nir/nir_opt_if.c
 >> index dacf2d6c667..11c6693d302 100644
 >> --- a/src/compiler/nir/nir_opt_if.c
 >> +++ b/src/compiler/nir/nir_opt_if.c
 >> @@ -369,6 +369,76 @@ opt_if_loop_terminator(nir_if *nif)
 >>      return true;
 >>   }
 >>
 >> +static void
 >> +replace_if_condition_use_with_const(nir_src *use, void *mem_ctx,
 >> +                                    nir_cursor cursor, unsigned
nir_boolean,
 >> +                                    bool if_condition)
 >> +{
 >> +   /* Create const */
 >> +   nir_load_const_instr *load =
nir_load_const_instr_create(mem_ctx, 1, 32);


The one actual use of mem_ctx is right here.

 >> +   load->value.u32[0] = nir_boolean;
 >> +   nir_instr_insert(cursor, >instr);
 >> +
 >> +   /* Rewrite use to use const */
 >> +   nir_src new_src = nir_src_for_ssa(>def);
 >> +
 >> +   if (if_condition)
 >> +      nir_if_rewrite_condition(use->parent_if, new_src);
 >> +   else
 >> +      nir_instr_rewrite_src(use->parent_instr, use, new_src);
 >> +}
 >> +
 >> +static bool
 >> +evaluate_if_condition(nir_if *nif, nir_cursor cursor, uint32_t
*value)
 >> +{
 >> +   nir_block *use_block = nir_cursor_current_block(cursor);
 >> +   if (nir_block_dominates(nir_if_first_then_block(nif),
use_block)) {
 >> +      *value = NIR_TRUE;
 >> +      return true;
 >> +   } else if (nir_block_dominates(nir_if_first_else_block(nif),
use_block)) {
 >> +      *value = NIR_FALSE;
 >> +      return true;
 >> +   } else {
 >> +      return false;
 >> +   }
 >> +}
 >> +
 >> +static bool
 >> +evaluate_condition_use(nir_if *nif, nir_src *use_src, void
*mem_ctx,
 >> +                       bool is_if_condition)

Re: [Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

2018-08-29 Thread Jason Ekstrand

On Wed, Aug 29, 2018 at 9:45 PM Timothy Arceri 
wrote:

> On 30/08/18 10:57, Ian Romanick wrote:
> > On 08/27/2018 02:08 AM, Timothy Arceri wrote:
> >> Since we know what side of the branch we ended up on we can just
> >> replace the use with a constant.
> >>
> >> All the spill changes in shader-db are from Dolphin uber shaders,
> >> despite some small regressions the change is clearly positive.
> >>
> >> V2: insert new constant after any phis in the
> >>  use->parent_instr->type == nir_instr_type_phi path.
> >>
> >> v3:
> >>   - use nir_after_block_before_jump() for inserting const
> >>   - check dominance of phi uses correctly
> >>
> >> v4:
> >>   - create some helpers as suggested by Jason.
> >>
> >> shader-db results IVB:
> >>
> >> total instructions in shared programs: 201 -> 9993483 (-0.06%)
> >> instructions in affected programs: 163235 -> 157517 (-3.50%)
> >> helped: 132
> >> HURT: 2
> >>
> >> total cycles in shared programs: 231670754 -> 219476091 (-5.26%)
> >> cycles in affected programs: 143424120 -> 131229457 (-8.50%)
> >> helped: 115
> >> HURT: 24
> >>
> >> total spills in shared programs: 4383 -> 4370 (-0.30%)
> >> spills in affected programs: 1656 -> 1643 (-0.79%)
> >> helped: 9
> >> HURT: 18
> >>
> >> total fills in shared programs: 4610 -> 4581 (-0.63%)
> >> fills in affected programs: 374 -> 345 (-7.75%)
> >> helped: 6
> >> HURT: 0
> >> ---
> >>   src/compiler/nir/nir.h|  22 +++
> >>   src/compiler/nir/nir_opt_if.c | 113 ++
> >>   2 files changed, 135 insertions(+)
> >>
> >> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> >> index 009a6d60371..0caacd30173 100644
> >> --- a/src/compiler/nir/nir.h
> >> +++ b/src/compiler/nir/nir.h
> >> @@ -2331,6 +2331,28 @@ nir_after_block_before_jump(nir_block *block)
> >>  }
> >>   }
> >>
> >> +static inline nir_cursor
> >> +nir_before_src(nir_src *src, bool is_if_condition)
> >> +{
> >> +   if (is_if_condition) {
> >> +  nir_block *prev_block =
> >> +
>  nir_cf_node_as_block(nir_cf_node_prev(>parent_if->cf_node));
> >> +  assert(!nir_block_ends_in_jump(prev_block));
> >> +  return nir_after_block(prev_block);
> >> +   } else if (src->parent_instr->type == nir_instr_type_phi) {
> >> +  nir_phi_instr *cond_phi = nir_instr_as_phi(src->parent_instr);
> >> +  nir_foreach_phi_src(phi_src, cond_phi) {
> >> + if (phi_src->src.ssa == src->ssa) {
> >> +return nir_after_block_before_jump(phi_src->pred);
> >> + }
> >> +  }
> >> +
> >> +  unreachable("failed to find phi src");
> >> +   } else {
> >> +  return nir_before_instr(src->parent_instr);
> >> +   }
> >> +}
> >> +
> >>   static inline nir_cursor
> >>   nir_before_cf_node(nir_cf_node *node)
> >>   {
> >> diff --git a/src/compiler/nir/nir_opt_if.c
> b/src/compiler/nir/nir_opt_if.c
> >> index dacf2d6c667..11c6693d302 100644
> >> --- a/src/compiler/nir/nir_opt_if.c
> >> +++ b/src/compiler/nir/nir_opt_if.c
> >> @@ -369,6 +369,76 @@ opt_if_loop_terminator(nir_if *nif)
> >>  return true;
> >>   }
> >>
> >> +static void
> >> +replace_if_condition_use_with_const(nir_src *use, void *mem_ctx,
> >> +nir_cursor cursor, unsigned
> nir_boolean,
> >> +bool if_condition)
> >> +{
> >> +   /* Create const */
> >> +   nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx,
> 1, 32);
>

The one actual use of mem_ctx is right here.


> >> +   load->value.u32[0] = nir_boolean;
> >> +   nir_instr_insert(cursor, >instr);
> >> +
> >> +   /* Rewrite use to use const */
> >> +   nir_src new_src = nir_src_for_ssa(>def);
> >> +
> >> +   if (if_condition)
> >> +  nir_if_rewrite_condition(use->parent_if, new_src);
> >> +   else
> >> +  nir_instr_rewrite_src(use->parent_instr, use, new_src);
> >> +}
> >> +
> >> +static bool
> >> +evaluate_if_condition(nir_if *nif, nir_cursor cursor, uint32_t *value)
> >> +{
> >> +   nir_block *use_block = nir_cursor_current_block(cursor);
> >> +   if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
> >> +  *value = NIR_TRUE;
> >> +  return true;
> >> +   } else if (nir_block_dominates(nir_if_first_else_block(nif),
> use_block)) {
> >> +  *value = NIR_FALSE;
> >> +  return true;
> >> +   } else {
> >> +  return false;
> >> +   }
> >> +}
> >> +
> >> +static bool
> >> +evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,
> >> +   bool is_if_condition)
> >> +{
> >> +   bool progress = false;
> >> +
> >> +   uint32_t value;
> >> +   nir_cursor cursor = nir_before_src(use_src, is_if_condition);
> >> +   if (evaluate_if_condition(nif, cursor, )) {
> >> +  replace_if_condition_use_with_const(use_src, mem_ctx, cursor,
> value,
> >> +  is_if_condition);
> >> +  progress = true;
> >> +   }
> >> +
> >> +   return progress;
> >> +}
> >> +
> >> +static bool
> >>

Re: [Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

2018-08-29 Thread Timothy Arceri


On 30/08/18 10:57, Ian Romanick wrote:

On 08/27/2018 02:08 AM, Timothy Arceri wrote:

Since we know what side of the branch we ended up on we can just
replace the use with a constant.

All the spill changes in shader-db are from Dolphin uber shaders,
despite some small regressions the change is clearly positive.

V2: insert new constant after any phis in the
 use->parent_instr->type == nir_instr_type_phi path.

v3:
  - use nir_after_block_before_jump() for inserting const
  - check dominance of phi uses correctly

v4:
  - create some helpers as suggested by Jason.

shader-db results IVB:

total instructions in shared programs: 201 -> 9993483 (-0.06%)
instructions in affected programs: 163235 -> 157517 (-3.50%)
helped: 132
HURT: 2

total cycles in shared programs: 231670754 -> 219476091 (-5.26%)
cycles in affected programs: 143424120 -> 131229457 (-8.50%)
helped: 115
HURT: 24

total spills in shared programs: 4383 -> 4370 (-0.30%)
spills in affected programs: 1656 -> 1643 (-0.79%)
helped: 9
HURT: 18

total fills in shared programs: 4610 -> 4581 (-0.63%)
fills in affected programs: 374 -> 345 (-7.75%)
helped: 6
HURT: 0
---
  src/compiler/nir/nir.h|  22 +++
  src/compiler/nir/nir_opt_if.c | 113 ++
  2 files changed, 135 insertions(+)

diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index 009a6d60371..0caacd30173 100644
--- a/src/compiler/nir/nir.h
+++ b/src/compiler/nir/nir.h
@@ -2331,6 +2331,28 @@ nir_after_block_before_jump(nir_block *block)
 }
  }
  
+static inline nir_cursor

+nir_before_src(nir_src *src, bool is_if_condition)
+{
+   if (is_if_condition) {
+  nir_block *prev_block =
+ nir_cf_node_as_block(nir_cf_node_prev(>parent_if->cf_node));
+  assert(!nir_block_ends_in_jump(prev_block));
+  return nir_after_block(prev_block);
+   } else if (src->parent_instr->type == nir_instr_type_phi) {
+  nir_phi_instr *cond_phi = nir_instr_as_phi(src->parent_instr);
+  nir_foreach_phi_src(phi_src, cond_phi) {
+ if (phi_src->src.ssa == src->ssa) {
+return nir_after_block_before_jump(phi_src->pred);
+ }
+  }
+
+  unreachable("failed to find phi src");
+   } else {
+  return nir_before_instr(src->parent_instr);
+   }
+}
+
  static inline nir_cursor
  nir_before_cf_node(nir_cf_node *node)
  {
diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
index dacf2d6c667..11c6693d302 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -369,6 +369,76 @@ opt_if_loop_terminator(nir_if *nif)
 return true;
  }
  
+static void

+replace_if_condition_use_with_const(nir_src *use, void *mem_ctx,
+nir_cursor cursor, unsigned nir_boolean,
+bool if_condition)
+{
+   /* Create const */
+   nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx, 1, 32);
+   load->value.u32[0] = nir_boolean;
+   nir_instr_insert(cursor, >instr);
+
+   /* Rewrite use to use const */
+   nir_src new_src = nir_src_for_ssa(>def);
+
+   if (if_condition)
+  nir_if_rewrite_condition(use->parent_if, new_src);
+   else
+  nir_instr_rewrite_src(use->parent_instr, use, new_src);
+}
+
+static bool
+evaluate_if_condition(nir_if *nif, nir_cursor cursor, uint32_t *value)
+{
+   nir_block *use_block = nir_cursor_current_block(cursor);
+   if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
+  *value = NIR_TRUE;
+  return true;
+   } else if (nir_block_dominates(nir_if_first_else_block(nif), use_block)) {
+  *value = NIR_FALSE;
+  return true;
+   } else {
+  return false;
+   }
+}
+
+static bool
+evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,
+   bool is_if_condition)
+{
+   bool progress = false;
+
+   uint32_t value;
+   nir_cursor cursor = nir_before_src(use_src, is_if_condition);
+   if (evaluate_if_condition(nif, cursor, )) {
+  replace_if_condition_use_with_const(use_src, mem_ctx, cursor, value,
+  is_if_condition);
+  progress = true;
+   }
+
+   return progress;
+}
+
+static bool
+opt_if_evaluate_condition_use(nir_if *nif, void *mem_ctx)
+{
+   bool progress = false;
+
+   /* Evaluate any uses of the if condition inside the if branches */
+   assert(nif->condition.is_ssa);
+   nir_foreach_use_safe(use_src, nif->condition.ssa) {
+  progress |= evaluate_condition_use(nif, use_src, mem_ctx, false);
+   }
+
+   nir_foreach_if_use_safe(use_src, nif->condition.ssa) {
+  if (use_src->parent_if != nif)
+ progress |= evaluate_condition_use(nif, use_src, mem_ctx, true);
+   }
+
+   return progress;
+}
+
  static bool
  opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
  {
@@ -402,6 +472,41 @@ opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
 return progress;
  }
  
+/**

+ * These optimisations depend on nir_metadata_block_index and

Re: [Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

2018-08-29 Thread Jason Ekstrand

On Wed, Aug 29, 2018 at 7:58 PM Ian Romanick  wrote:

> On 08/27/2018 02:08 AM, Timothy Arceri wrote:
> > Since we know what side of the branch we ended up on we can just
> > replace the use with a constant.
> >
> > All the spill changes in shader-db are from Dolphin uber shaders,
> > despite some small regressions the change is clearly positive.
> >
> > V2: insert new constant after any phis in the
> > use->parent_instr->type == nir_instr_type_phi path.
> >
> > v3:
> >  - use nir_after_block_before_jump() for inserting const
> >  - check dominance of phi uses correctly
> >
> > v4:
> >  - create some helpers as suggested by Jason.
> >
> > shader-db results IVB:
> >
> > total instructions in shared programs: 201 -> 9993483 (-0.06%)
> > instructions in affected programs: 163235 -> 157517 (-3.50%)
> > helped: 132
> > HURT: 2
> >
> > total cycles in shared programs: 231670754 -> 219476091 (-5.26%)
> > cycles in affected programs: 143424120 -> 131229457 (-8.50%)
> > helped: 115
> > HURT: 24
> >
> > total spills in shared programs: 4383 -> 4370 (-0.30%)
> > spills in affected programs: 1656 -> 1643 (-0.79%)
> > helped: 9
> > HURT: 18
> >
> > total fills in shared programs: 4610 -> 4581 (-0.63%)
> > fills in affected programs: 374 -> 345 (-7.75%)
> > helped: 6
> > HURT: 0
> > ---
> >  src/compiler/nir/nir.h|  22 +++
> >  src/compiler/nir/nir_opt_if.c | 113 ++
> >  2 files changed, 135 insertions(+)
> >
> > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> > index 009a6d60371..0caacd30173 100644
> > --- a/src/compiler/nir/nir.h
> > +++ b/src/compiler/nir/nir.h
> > @@ -2331,6 +2331,28 @@ nir_after_block_before_jump(nir_block *block)
> > }
> >  }
> >
> > +static inline nir_cursor
> > +nir_before_src(nir_src *src, bool is_if_condition)
> > +{
> > +   if (is_if_condition) {
> > +  nir_block *prev_block =
> > +
>  nir_cf_node_as_block(nir_cf_node_prev(>parent_if->cf_node));
> > +  assert(!nir_block_ends_in_jump(prev_block));
> > +  return nir_after_block(prev_block);
> > +   } else if (src->parent_instr->type == nir_instr_type_phi) {
> > +  nir_phi_instr *cond_phi = nir_instr_as_phi(src->parent_instr);
> > +  nir_foreach_phi_src(phi_src, cond_phi) {
> > + if (phi_src->src.ssa == src->ssa) {
> > +return nir_after_block_before_jump(phi_src->pred);
> > + }
> > +  }
> > +
> > +  unreachable("failed to find phi src");
> > +   } else {
> > +  return nir_before_instr(src->parent_instr);
> > +   }
> > +}
> > +
> >  static inline nir_cursor
> >  nir_before_cf_node(nir_cf_node *node)
> >  {
> > diff --git a/src/compiler/nir/nir_opt_if.c
> b/src/compiler/nir/nir_opt_if.c
> > index dacf2d6c667..11c6693d302 100644
> > --- a/src/compiler/nir/nir_opt_if.c
> > +++ b/src/compiler/nir/nir_opt_if.c
> > @@ -369,6 +369,76 @@ opt_if_loop_terminator(nir_if *nif)
> > return true;
> >  }
> >
> > +static void
> > +replace_if_condition_use_with_const(nir_src *use, void *mem_ctx,
> > +nir_cursor cursor, unsigned
> nir_boolean,
> > +bool if_condition)
> > +{
> > +   /* Create const */
> > +   nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx, 1,
> 32);
> > +   load->value.u32[0] = nir_boolean;
> > +   nir_instr_insert(cursor, >instr);
> > +
> > +   /* Rewrite use to use const */
> > +   nir_src new_src = nir_src_for_ssa(>def);
> > +
> > +   if (if_condition)
> > +  nir_if_rewrite_condition(use->parent_if, new_src);
> > +   else
> > +  nir_instr_rewrite_src(use->parent_instr, use, new_src);
> > +}
> > +
> > +static bool
> > +evaluate_if_condition(nir_if *nif, nir_cursor cursor, uint32_t *value)
> > +{
> > +   nir_block *use_block = nir_cursor_current_block(cursor);
> > +   if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
> > +  *value = NIR_TRUE;
> > +  return true;
> > +   } else if (nir_block_dominates(nir_if_first_else_block(nif),
> use_block)) {
> > +  *value = NIR_FALSE;
> > +  return true;
> > +   } else {
> > +  return false;
> > +   }
> > +}
> > +
> > +static bool
> > +evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,
> > +   bool is_if_condition)
> > +{
> > +   bool progress = false;
> > +
> > +   uint32_t value;
> > +   nir_cursor cursor = nir_before_src(use_src, is_if_condition);
> > +   if (evaluate_if_condition(nif, cursor, )) {
> > +  replace_if_condition_use_with_const(use_src, mem_ctx, cursor,
> value,
> > +  is_if_condition);
> > +  progress = true;
> > +   }
> > +
> > +   return progress;
> > +}
> > +
> > +static bool
> > +opt_if_evaluate_condition_use(nir_if *nif, void *mem_ctx)
> > +{
> > +   bool progress = false;
> > +
> > +   /* Evaluate any uses of the if condition inside the if branches */
> > +   assert(nif->condition.is_ssa);
> > +   nir_foreach_use_safe(use_src,

Re: [Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

2018-08-29 Thread Ian Romanick

On 08/27/2018 02:08 AM, Timothy Arceri wrote:
> Since we know what side of the branch we ended up on we can just
> replace the use with a constant.
> 
> All the spill changes in shader-db are from Dolphin uber shaders,
> despite some small regressions the change is clearly positive.
> 
> V2: insert new constant after any phis in the
> use->parent_instr->type == nir_instr_type_phi path.
> 
> v3:
>  - use nir_after_block_before_jump() for inserting const
>  - check dominance of phi uses correctly
> 
> v4:
>  - create some helpers as suggested by Jason.
> 
> shader-db results IVB:
> 
> total instructions in shared programs: 201 -> 9993483 (-0.06%)
> instructions in affected programs: 163235 -> 157517 (-3.50%)
> helped: 132
> HURT: 2
> 
> total cycles in shared programs: 231670754 -> 219476091 (-5.26%)
> cycles in affected programs: 143424120 -> 131229457 (-8.50%)
> helped: 115
> HURT: 24
> 
> total spills in shared programs: 4383 -> 4370 (-0.30%)
> spills in affected programs: 1656 -> 1643 (-0.79%)
> helped: 9
> HURT: 18
> 
> total fills in shared programs: 4610 -> 4581 (-0.63%)
> fills in affected programs: 374 -> 345 (-7.75%)
> helped: 6
> HURT: 0
> ---
>  src/compiler/nir/nir.h|  22 +++
>  src/compiler/nir/nir_opt_if.c | 113 ++
>  2 files changed, 135 insertions(+)
> 
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index 009a6d60371..0caacd30173 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -2331,6 +2331,28 @@ nir_after_block_before_jump(nir_block *block)
> }
>  }
>  
> +static inline nir_cursor
> +nir_before_src(nir_src *src, bool is_if_condition)
> +{
> +   if (is_if_condition) {
> +  nir_block *prev_block =
> + nir_cf_node_as_block(nir_cf_node_prev(>parent_if->cf_node));
> +  assert(!nir_block_ends_in_jump(prev_block));
> +  return nir_after_block(prev_block);
> +   } else if (src->parent_instr->type == nir_instr_type_phi) {
> +  nir_phi_instr *cond_phi = nir_instr_as_phi(src->parent_instr);
> +  nir_foreach_phi_src(phi_src, cond_phi) {
> + if (phi_src->src.ssa == src->ssa) {
> +return nir_after_block_before_jump(phi_src->pred);
> + }
> +  }
> +
> +  unreachable("failed to find phi src");
> +   } else {
> +  return nir_before_instr(src->parent_instr);
> +   }
> +}
> +
>  static inline nir_cursor
>  nir_before_cf_node(nir_cf_node *node)
>  {
> diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
> index dacf2d6c667..11c6693d302 100644
> --- a/src/compiler/nir/nir_opt_if.c
> +++ b/src/compiler/nir/nir_opt_if.c
> @@ -369,6 +369,76 @@ opt_if_loop_terminator(nir_if *nif)
> return true;
>  }
>  
> +static void
> +replace_if_condition_use_with_const(nir_src *use, void *mem_ctx,
> +nir_cursor cursor, unsigned nir_boolean,
> +bool if_condition)
> +{
> +   /* Create const */
> +   nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx, 1, 32);
> +   load->value.u32[0] = nir_boolean;
> +   nir_instr_insert(cursor, >instr);
> +
> +   /* Rewrite use to use const */
> +   nir_src new_src = nir_src_for_ssa(>def);
> +
> +   if (if_condition)
> +  nir_if_rewrite_condition(use->parent_if, new_src);
> +   else
> +  nir_instr_rewrite_src(use->parent_instr, use, new_src);
> +}
> +
> +static bool
> +evaluate_if_condition(nir_if *nif, nir_cursor cursor, uint32_t *value)
> +{
> +   nir_block *use_block = nir_cursor_current_block(cursor);
> +   if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
> +  *value = NIR_TRUE;
> +  return true;
> +   } else if (nir_block_dominates(nir_if_first_else_block(nif), use_block)) {
> +  *value = NIR_FALSE;
> +  return true;
> +   } else {
> +  return false;
> +   }
> +}
> +
> +static bool
> +evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,
> +   bool is_if_condition)
> +{
> +   bool progress = false;
> +
> +   uint32_t value;
> +   nir_cursor cursor = nir_before_src(use_src, is_if_condition);
> +   if (evaluate_if_condition(nif, cursor, )) {
> +  replace_if_condition_use_with_const(use_src, mem_ctx, cursor, value,
> +  is_if_condition);
> +  progress = true;
> +   }
> +
> +   return progress;
> +}
> +
> +static bool
> +opt_if_evaluate_condition_use(nir_if *nif, void *mem_ctx)
> +{
> +   bool progress = false;
> +
> +   /* Evaluate any uses of the if condition inside the if branches */
> +   assert(nif->condition.is_ssa);
> +   nir_foreach_use_safe(use_src, nif->condition.ssa) {
> +  progress |= evaluate_condition_use(nif, use_src, mem_ctx, false);
> +   }
> +
> +   nir_foreach_if_use_safe(use_src, nif->condition.ssa) {
> +  if (use_src->parent_if != nif)
> + progress |= evaluate_condition_use(nif, use_src, mem_ctx, true);
> +   }
> +
> +   return progress;
> +}
> +
>  static

Re: [Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

2018-08-28 Thread Jason Ekstrand

On Mon, Aug 27, 2018 at 4:09 AM Timothy Arceri 
wrote:

> Since we know what side of the branch we ended up on we can just
> replace the use with a constant.
>
> All the spill changes in shader-db are from Dolphin uber shaders,
> despite some small regressions the change is clearly positive.
>
> V2: insert new constant after any phis in the
> use->parent_instr->type == nir_instr_type_phi path.
>
> v3:
>  - use nir_after_block_before_jump() for inserting const
>  - check dominance of phi uses correctly
>
> v4:
>  - create some helpers as suggested by Jason.
>
> shader-db results IVB:
>
> total instructions in shared programs: 201 -> 9993483 (-0.06%)
> instructions in affected programs: 163235 -> 157517 (-3.50%)
> helped: 132
> HURT: 2
>
> total cycles in shared programs: 231670754 -> 219476091 (-5.26%)
> cycles in affected programs: 143424120 -> 131229457 (-8.50%)
> helped: 115
> HURT: 24
>
> total spills in shared programs: 4383 -> 4370 (-0.30%)
> spills in affected programs: 1656 -> 1643 (-0.79%)
> helped: 9
> HURT: 18
>
> total fills in shared programs: 4610 -> 4581 (-0.63%)
> fills in affected programs: 374 -> 345 (-7.75%)
> helped: 6
> HURT: 0
> ---
>  src/compiler/nir/nir.h|  22 +++
>  src/compiler/nir/nir_opt_if.c | 113 ++
>  2 files changed, 135 insertions(+)
>
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index 009a6d60371..0caacd30173 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -2331,6 +2331,28 @@ nir_after_block_before_jump(nir_block *block)
> }
>  }
>
> +static inline nir_cursor
> +nir_before_src(nir_src *src, bool is_if_condition)
> +{
> +   if (is_if_condition) {
> +  nir_block *prev_block =
> + nir_cf_node_as_block(nir_cf_node_prev(>parent_if->cf_node));
> +  assert(!nir_block_ends_in_jump(prev_block));
> +  return nir_after_block(prev_block);
> +   } else if (src->parent_instr->type == nir_instr_type_phi) {
> +  nir_phi_instr *cond_phi = nir_instr_as_phi(src->parent_instr);
> +  nir_foreach_phi_src(phi_src, cond_phi) {
> + if (phi_src->src.ssa == src->ssa) {
> +return nir_after_block_before_jump(phi_src->pred);
> + }
> +  }
>

This works.  Some sort of container_of would be more efficient.  I guess
that wouldn't work if the source was a register indirect on a phi src but I
don't think that's valid.


> +
> +  unreachable("failed to find phi src");
>

If the source isn't a phi src (and container_of would fail) hitting an
unreachable is really nasty.  I don't think we will hit this case but I
also think that if we do, container_of is actually the safer option as
weird as that sounds.  That said, we should do something to ensure that we
get an assertion failure if it ever isn't the source of a phi.  What you
did above works.  If we did container_of, we should add an assert loop
somehow.


> +   } else {
> +  return nir_before_instr(src->parent_instr);
> +   }
> +}
> +
>  static inline nir_cursor
>  nir_before_cf_node(nir_cf_node *node)
>  {
> diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
> index dacf2d6c667..11c6693d302 100644
> --- a/src/compiler/nir/nir_opt_if.c
> +++ b/src/compiler/nir/nir_opt_if.c
> @@ -369,6 +369,76 @@ opt_if_loop_terminator(nir_if *nif)
> return true;
>  }
>
> +static void
> +replace_if_condition_use_with_const(nir_src *use, void *mem_ctx,
> +nir_cursor cursor, unsigned
> nir_boolean,
>

Mind making nir_boolean a uint32_t?

With that (and regardless of what way we go with container_of above), this
patch is

Reviewed-by: Jason Ekstrand 


> +bool if_condition)
> +{
> +   /* Create const */
> +   nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx, 1,
> 32);
> +   load->value.u32[0] = nir_boolean;
> +   nir_instr_insert(cursor, >instr);
> +
> +   /* Rewrite use to use const */
> +   nir_src new_src = nir_src_for_ssa(>def);
> +
> +   if (if_condition)
> +  nir_if_rewrite_condition(use->parent_if, new_src);
> +   else
> +  nir_instr_rewrite_src(use->parent_instr, use, new_src);
> +}
> +
> +static bool
> +evaluate_if_condition(nir_if *nif, nir_cursor cursor, uint32_t *value)
> +{
> +   nir_block *use_block = nir_cursor_current_block(cursor);
> +   if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
> +  *value = NIR_TRUE;
> +  return true;
> +   } else if (nir_block_dominates(nir_if_first_else_block(nif),
> use_block)) {
> +  *value = NIR_FALSE;
> +  return true;
> +   } else {
> +  return false;
> +   }
> +}
> +
> +static bool
> +evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,
> +   bool is_if_condition)
> +{
> +   bool progress = false;
> +
> +   uint32_t value;
> +   nir_cursor cursor = nir_before_src(use_src, is_if_condition);
> +   if (evaluate_if_condition(nif, cursor, )) {
> +

[Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

2018-08-27 Thread Timothy Arceri

Since we know what side of the branch we ended up on we can just
replace the use with a constant.

All the spill changes in shader-db are from Dolphin uber shaders,
despite some small regressions the change is clearly positive.

V2: insert new constant after any phis in the
use->parent_instr->type == nir_instr_type_phi path.

v3:
 - use nir_after_block_before_jump() for inserting const
 - check dominance of phi uses correctly

v4:
 - create some helpers as suggested by Jason.

shader-db results IVB:

total instructions in shared programs: 201 -> 9993483 (-0.06%)
instructions in affected programs: 163235 -> 157517 (-3.50%)
helped: 132
HURT: 2

total cycles in shared programs: 231670754 -> 219476091 (-5.26%)
cycles in affected programs: 143424120 -> 131229457 (-8.50%)
helped: 115
HURT: 24

total spills in shared programs: 4383 -> 4370 (-0.30%)
spills in affected programs: 1656 -> 1643 (-0.79%)
helped: 9
HURT: 18

total fills in shared programs: 4610 -> 4581 (-0.63%)
fills in affected programs: 374 -> 345 (-7.75%)
helped: 6
HURT: 0
---
 src/compiler/nir/nir.h|  22 +++
 src/compiler/nir/nir_opt_if.c | 113 ++
 2 files changed, 135 insertions(+)

diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index 009a6d60371..0caacd30173 100644
--- a/src/compiler/nir/nir.h
+++ b/src/compiler/nir/nir.h
@@ -2331,6 +2331,28 @@ nir_after_block_before_jump(nir_block *block)
}
 }
 
+static inline nir_cursor
+nir_before_src(nir_src *src, bool is_if_condition)
+{
+   if (is_if_condition) {
+  nir_block *prev_block =
+ nir_cf_node_as_block(nir_cf_node_prev(>parent_if->cf_node));
+  assert(!nir_block_ends_in_jump(prev_block));
+  return nir_after_block(prev_block);
+   } else if (src->parent_instr->type == nir_instr_type_phi) {
+  nir_phi_instr *cond_phi = nir_instr_as_phi(src->parent_instr);
+  nir_foreach_phi_src(phi_src, cond_phi) {
+ if (phi_src->src.ssa == src->ssa) {
+return nir_after_block_before_jump(phi_src->pred);
+ }
+  }
+
+  unreachable("failed to find phi src");
+   } else {
+  return nir_before_instr(src->parent_instr);
+   }
+}
+
 static inline nir_cursor
 nir_before_cf_node(nir_cf_node *node)
 {
diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
index dacf2d6c667..11c6693d302 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -369,6 +369,76 @@ opt_if_loop_terminator(nir_if *nif)
return true;
 }
 
+static void
+replace_if_condition_use_with_const(nir_src *use, void *mem_ctx,
+nir_cursor cursor, unsigned nir_boolean,
+bool if_condition)
+{
+   /* Create const */
+   nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx, 1, 32);
+   load->value.u32[0] = nir_boolean;
+   nir_instr_insert(cursor, >instr);
+
+   /* Rewrite use to use const */
+   nir_src new_src = nir_src_for_ssa(>def);
+
+   if (if_condition)
+  nir_if_rewrite_condition(use->parent_if, new_src);
+   else
+  nir_instr_rewrite_src(use->parent_instr, use, new_src);
+}
+
+static bool
+evaluate_if_condition(nir_if *nif, nir_cursor cursor, uint32_t *value)
+{
+   nir_block *use_block = nir_cursor_current_block(cursor);
+   if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
+  *value = NIR_TRUE;
+  return true;
+   } else if (nir_block_dominates(nir_if_first_else_block(nif), use_block)) {
+  *value = NIR_FALSE;
+  return true;
+   } else {
+  return false;
+   }
+}
+
+static bool
+evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,
+   bool is_if_condition)
+{
+   bool progress = false;
+
+   uint32_t value;
+   nir_cursor cursor = nir_before_src(use_src, is_if_condition);
+   if (evaluate_if_condition(nif, cursor, )) {
+  replace_if_condition_use_with_const(use_src, mem_ctx, cursor, value,
+  is_if_condition);
+  progress = true;
+   }
+
+   return progress;
+}
+
+static bool
+opt_if_evaluate_condition_use(nir_if *nif, void *mem_ctx)
+{
+   bool progress = false;
+
+   /* Evaluate any uses of the if condition inside the if branches */
+   assert(nif->condition.is_ssa);
+   nir_foreach_use_safe(use_src, nif->condition.ssa) {
+  progress |= evaluate_condition_use(nif, use_src, mem_ctx, false);
+   }
+
+   nir_foreach_if_use_safe(use_src, nif->condition.ssa) {
+  if (use_src->parent_if != nif)
+ progress |= evaluate_condition_use(nif, use_src, mem_ctx, true);
+   }
+
+   return progress;
+}
+
 static bool
 opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
 {
@@ -402,6 +472,41 @@ opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
return progress;
 }
 
+/**
+ * These optimisations depend on nir_metadata_block_index and therefore must
+ * not do anything to cause the metadata to become invalid.
+ */
+static bool

Re: [Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

Re: [Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

Re: [Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

Re: [Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

Re: [Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

Re: [Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

[Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

7 matches

Site Navigation

Mail list logo

Footer information