<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jan 4, 2017 at 9:31 PM, Timothy Arceri <span dir="ltr"><<a href="mailto:timothy.arceri@collabora.com" target="_blank">timothy.arceri@collabora.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">There was a bit to take in here but it seems ok to me. I've made a<br>
bunch of trivial suggestions/comments below otherwise:<br>
<br>
Reviewed-by: Timothy Arceri <<a href="mailto:timothy.arceri@collabora.com">timothy.arceri@collabora.com</a>><br>
<div><div class="gmail-h5"><br>
On Mon, 2016-12-12 at 19:39 -0800, Jason Ekstrand wrote:<br>
> ---<br>
>  src/compiler/Makefile.<wbr>sources             |   1 +<br>
>  src/compiler/nir/nir.h       <wbr>             |   2 +<br>
>  src/compiler/nir/nir_opt_<wbr>copy_prop_vars.c | 799<br>
> ++++++++++++++++++++++++++++++<br>
>  3 files changed, 802 insertions(+)<br>
>  create mode 100644 src/compiler/nir/nir_opt_copy_<wbr>prop_vars.c<br>
><br>
> diff --git a/src/compiler/Makefile.<wbr>sources<br>
> b/src/compiler/Makefile.<wbr>sources<br>
> index 17b15de..09b4105 100644<br>
> --- a/src/compiler/Makefile.<wbr>sources<br>
> +++ b/src/compiler/Makefile.<wbr>sources<br>
> @@ -231,6 +231,7 @@ NIR_FILES = \<br>
>       nir/nir_normalize_cubemap_<wbr>coords.c \<br>
>       nir/nir_opt_conditional_<wbr>discard.c \<br>
>       nir/nir_opt_constant_folding.c \<br>
> +     nir/nir_opt_copy_prop_vars.c \<br>
>       nir/nir_opt_copy_propagate.c \<br>
>       nir/nir_opt_cse.c \<br>
>       nir/nir_opt_dce.c \<br>
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h<br>
> index 544d4ba..27ef633 100644<br>
> --- a/src/compiler/nir/nir.h<br>
> +++ b/src/compiler/nir/nir.h<br>
> @@ -2489,6 +2489,8 @@ bool nir_opt_global_to_local(nir_<wbr>shader<br>
> *shader);<br>
>  <br>
>  bool nir_copy_prop(nir_shader *shader);<br>
>  <br>
> +bool nir_opt_copy_prop_vars(nir_<wbr>shader *shader);<br>
> +<br>
>  bool nir_opt_cse(nir_shader *shader);<br>
>  <br>
>  bool nir_opt_dce(nir_shader *shader);<br>
> diff --git a/src/compiler/nir/nir_opt_<wbr>copy_prop_vars.c<br>
> b/src/compiler/nir/nir_opt_<wbr>copy_prop_vars.c<br>
> new file mode 100644<br>
> index 0000000..728e476<br>
> --- /dev/null<br>
> +++ b/src/compiler/nir/nir_opt_<wbr>copy_prop_vars.c<br>
> @@ -0,0 +1,799 @@<br>
> +/*<br>
> + * Copyright © 2016 Intel Corporation<br>
> + *<br>
> + * Permission is hereby granted, free of charge, to any person<br>
> obtaining a<br>
> + * copy of this software and associated documentation files (the<br>
> "Software"),<br>
> + * to deal in the Software without restriction, including without<br>
> limitation<br>
> + * the rights to use, copy, modify, merge, publish, distribute,<br>
> sublicense,<br>
> + * and/or sell copies of the Software, and to permit persons to whom<br>
> the<br>
> + * Software is furnished to do so, subject to the following<br>
> conditions:<br>
> + *<br>
> + * The above copyright notice and this permission notice (including<br>
> the next<br>
> + * paragraph) shall be included in all copies or substantial<br>
> portions of the<br>
> + * Software.<br>
> + *<br>
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,<br>
> EXPRESS OR<br>
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF<br>
> MERCHANTABILITY,<br>
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO<br>
> EVENT SHALL<br>
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES<br>
> OR OTHER<br>
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,<br>
> ARISING<br>
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR<br>
> OTHER DEALINGS<br>
> + * IN THE SOFTWARE.<br>
> + */<br>
> +<br>
> +#include "nir.h"<br>
> +#include "nir_builder.h"<br>
> +<br>
> +/**<br>
> + * Variable-based copy propagation<br>
> + *<br>
> + * Normally, NIR trusts in SSA form for most of its copy-propagation<br>
> needs.<br>
> + * However, there are cases, especially when dealing with indirects,<br>
> where SSA<br>
> + * won't help you.  This pass is for those times.  Specifically, it<br>
> handles<br>
> + * the following things that the rest of NIR can't:<br>
> + *<br>
> + *  1) Copy-propagation on variables that have indirect<br>
> access.  This includes<br>
> + *     propagating from indirect stores into indirect loads.<br>
> + *<br>
> + *  2) Dead code elimination of store_var and copy_var intrinsics<br>
> based on<br>
> + *     killed destination values.<br>
> + *<br>
> + *  3) Removal of redundant load_var intrinsics.  We can't trust<br>
> regular CSE<br>
> + *     to do this because it isn't aware of variable writes that may<br>
> alias the<br>
> + *     value and make the former load invalid.<br>
> + *<br>
> + * Unfortunately, properly handling all of those cases makes this<br>
> path rather<br>
> + * complex.  In order to avoid additional complexity, this pass is<br>
> entirely<br>
> + * block-local.  If we tried to make it local the data-flow analysis<br>
> would<br>
> + * rapidly get out of hand.  Fortunately, for anything that is only<br>
> ever<br>
> + * accessed directly, we get SSA based copy-propagation which is<br>
> extremely<br>
> + * powerful so this isn't that great a loss.<br>
> + */<br>
> +<br>
> +struct value {<br>
> +   bool is_ssa;<br>
> +   union {<br>
> +      nir_ssa_def *ssa[4];<br>
> +      nir_deref_var *deref;<br>
> +   };<br>
> +};<br>
> +<br>
> +struct copy_entry {<br>
> +   struct list_head link;<br>
> +<br>
> +   nir_instr *store_instr[4];<br>
> +<br>
> +   unsigned comps_may_be_read;<br>
> +   struct value src;<br>
> +<br>
> +   nir_deref_var *dst;<br>
> +};<br>
> +<br>
> +struct copy_prop_var_state {<br>
> +   nir_shader *shader;<br>
> +<br>
> +   void *mem_ctx;<br>
> +<br>
> +   struct list_head copies;<br>
> +<br>
> +   /* We're going to be allocating and deleting a lot of copy<br>
> entries so we'll<br>
> +    * keep a free list to avoid thrashing malloc too badly.<br>
> +    */<br>
> +   struct list_head copy_free_list;<br>
> +<br>
> +   bool progress;<br>
> +};<br>
> +<br>
> +static struct copy_entry *<br>
> +copy_entry_create(struct copy_prop_var_state *state,<br>
> +                  nir_deref_<wbr>var *dst_deref)<br>
> +{<br>
> +   struct copy_entry *entry;<br>
> +   if (!list_empty(&state->copy_<wbr>free_list)) {<br>
> +      struct list_head *item = state->copy_free_list.next;<br>
> +      list_del(item);<br>
> +      entry = LIST_ENTRY(struct copy_entry, item, link);<br>
> +      memset(entry, 0, sizeof(*entry));<br>
> +   } else {<br>
> +      entry = rzalloc(state->mem_ctx, struct copy_entry);<br>
> +   }<br>
> +<br>
> +   entry->dst = dst_deref;<br>
> +   list_add(&entry->link, &state->copies);<br>
> +<br>
> +   return entry;<br>
> +}<br>
> +<br>
> +static void<br>
> +copy_entry_destroy(struct copy_prop_var_state *state, struct<br>
> copy_entry *entry)<br>
<br>
</div></div>No a huge deal but this doesn't really do what the name says. Maybe<br>
rename it?<br>
<br>
copy_entry_remove() ?<br></blockquote><div><br></div><div>Changed locally.  I couldn't come up with anything better.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-"><br>
> +{<br>
> +   list_del(&entry->link);<br>
> +   list_add(&entry->link, &state->copy_free_list);<br>
> +}<br>
> +<br>
> +enum deref_compare_result {<br>
> +   derefs_equal_bit = (1 << 0),<br>
> +   derefs_may_alias_bit = (1 << 1),<br>
> +   derefs_a_contains_b_bit = (1 << 2),<br>
> +   derefs_b_contains_a_bit = (1 << 3),<br>
> +};<br>
> +<br>
> +/** Returns true if the storage referrenced to by deref completely<br>
> contains<br>
> + * the storage referenced by sub.<br>
> + *<br>
> + * TODO: Should this go in core NIR?<br>
<br>
</span>I'm not sure if it should. But we should decide one way or the other<br>
not sure its very helpful to leave this TODO in place.<br></blockquote><div><br></div><div>Yeah... I stuck a TODO in there because I really liked the way it turned out and it was nice and general so it *could* be moved to into core NIR and used by other things.  Then again, no one else uses it right now.  I could go eather way.  Thoughts?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5">
> + */<br>
> +static enum deref_compare_result<br>
> +compare_derefs(nir_deref_var *a, nir_deref_var *b)<br>
> +{<br>
> +   if (a->var != b->var)<br>
> +      return 0;<br>
> +<br>
> +   /* Start off assuming they fully compare.  We ignore equality for<br>
> now.  In<br>
> +    * the end, we'll determine that by containment.<br>
> +    */<br>
> +   enum deref_compare_result result = derefs_may_alias_bit |<br>
> +                             <wbr>         derefs_a_contains_b_<wbr>bit |<br>
> +                             <wbr>         derefs_b_contains_a_<wbr>bit;<br>
> +<br>
> +   nir_deref *a_tail = &a->deref;<br>
> +   nir_deref *b_tail = &b->deref;<br>
> +   while (a_tail->child && b_tail->child) {<br>
> +      a_tail = a_tail->child;<br>
> +      b_tail = b_tail->child;<br>
> +<br>
> +      assert(a_tail->deref_<wbr>type == b_tail->deref_type);<br>
> +      switch (a_tail->deref_type) {<br>
> +      case nir_deref_type_array: {<br>
> +         nir_deref_array *a_arr = nir_deref_as_array(a_tail);<br>
> +         nir_deref_array *b_arr = nir_deref_as_array(b_tail);<br>
> +<br>
> +         if (a_arr->deref_array_type == nir_deref_array_type_direct<br>
> &&<br>
> +             b_arr->deref_<wbr>array_type == nir_deref_array_type_direct)<br>
> {<br>
> +            /* If they're both direct and have different offsets,<br>
> they<br>
> +             * don't even alias much less anything else.<br>
> +             */<br>
> +            if (a_arr->base_offset != b_arr->base_offset)<br>
> +               return 0;<br>
> +         } else if (a_arr->deref_array_type ==<br>
> nir_deref_array_type_wildcard) {<br>
> +            if (b_arr->deref_array_type !=<br>
> nir_deref_array_type_wildcard)<br>
> +               result &= ~derefs_b_contains_a_bit;<br>
> +         } else if (b_arr->deref_array_type ==<br>
> nir_deref_array_type_wildcard) {<br>
> +            if (a_arr->deref_array_type !=<br>
> nir_deref_array_type_wildcard)<br>
> +               result &= ~derefs_a_contains_b_bit;<br>
> +         } else if (a_arr->deref_array_type ==<br>
> nir_deref_array_type_indirect &&<br>
> +                    b_arr-><wbr>deref_array_type ==<br>
> nir_deref_array_type_indirect) {<br>
> +            assert(a_arr-><wbr>indirect.is_ssa && b_arr-<br>
> >indirect.is_ssa);<br>
> +            if (a_arr->indirect.ssa == b_arr->indirect.ssa) {<br>
> +               /* If they're different constant offsets from the<br>
> same indirect<br>
> +                * then they don't alias at all.<br>
> +                */<br>
> +               if (a_arr->base_offset != b_arr->base_offset)<br>
> +                  return 0;<br>
> +               /* Otherwise the indirect and base both match */<br>
> +            } else {<br>
> +               /* If they're have different indirect offsets then we<br>
> can't<br>
> +                * prove anything about containment.<br>
> +                */<br>
> +               result &= ~(derefs_a_contains_b_bit |<br>
> derefs_b_contains_a_bit);<br>
> +            }<br>
> +         } else {<br>
> +            /* In this case, one is indirect and the other direct so<br>
> we can't<br>
> +             * prove anything about containment.<br>
> +             */<br>
> +            result &= ~(derefs_a_contains_b_bit |<br>
> derefs_b_contains_a_bit);<br>
> +         }<br>
> +         break;<br>
> +      }<br>
> +<br>
> +      case nir_deref_type_struct: {<br>
> +         nir_deref_struct *a_struct = nir_deref_as_struct(a_tail);<br>
> +         nir_deref_struct *b_struct = nir_deref_as_struct(b_tail);<br>
> +<br>
> +         /* If they're different struct members, they don't even<br>
> alias */<br>
> +         if (a_struct->index != b_struct->index)<br>
> +            return 0;<br>
> +         break;<br>
> +      }<br>
> +<br>
> +      default:<br>
> +         unreachable("Invalid deref type");<br>
> +      }<br>
> +   }<br>
> +<br>
> +   /* If a is longer than b, then it can't contain b */<br>
> +   if (a_tail->child)<br>
> +      result &= ~derefs_a_contains_b_bit;<br>
> +   if (b_tail->child)<br>
> +      result &= ~derefs_b_contains_a_bit;<br>
> +<br>
> +   /* If a contains b and b contains a they must be equal. */<br>
> +   if ((result & derefs_a_contains_b_bit) && (result &<br>
> derefs_b_contains_a_bit))<br>
> +      result |= derefs_equal_bit;<br>
> +<br>
> +   return result;<br>
> +}<br>
> +<br>
> +static void<br>
> +remove_dead_writes(struct copy_prop_var_state *state,<br>
> +                   struct copy_entry *entry, unsigned write_mask)<br>
> +{<br>
> +   /* We're overwriting another entry.  Some of it's components may<br>
> not<br>
> +    * have been read yet and, if that's the case, we may be able to<br>
> delete<br>
> +    * some instructions but we have to be careful.<br>
> +    */<br>
> +   unsigned dead_comps = write_mask & ~entry->comps_may_be_read;<br>
> +   if (!dead_comps)<br>
> +      return;<br>
> +<br>
> +   for (unsigned i = 0; i < 4; i++) {<br>
> +      if (!(dead_comps & (1 << i)))<br>
> +         continue;<br>
<br>
</div></div>Up to you but we could replace this and the return above with:<br>
<br>
   unsigned mask = dead_comps;<br>
   while (mask) {<br>
      const int i = u_bit_scan(&mask);<br>
<br>
It's becoming an increasingly used pattern in Mesa and I find it<br>
cleaner but you may disagree.<br>
<div><div class="gmail-h5"><br>
> +<br>
> +      nir_instr *instr = entry->store_instr[i];<br>
> +<br>
> +      /* We may have already deleted it on a previous iteration */<br>
> +      if (!instr)<br>
> +         continue;<br>
> +<br>
> +      /* See if this instr is used anywhere that it's not dead */<br>
> +      bool keep = false;<br>
> +      for (unsigned j = 0; j < 4; j++) {<br>
> +         if (entry->store_instr[j] == instr) {<br>
> +            if (dead_comps & (1 << j)) {<br>
> +               entry->store_<wbr>instr[j] = NULL;<br>
> +            } else {<br>
> +               keep = true;<br>
> +            }<br>
> +         }<br>
> +      }<br>
> +<br>
> +      if (!keep) {<br>
> +         nir_instr_remove(<wbr>instr);<br>
> +         state->progress = true;<br>
> +      }<br>
> +   }<br>
> +}<br>
> +<br>
> +static struct copy_entry *<br>
> +lookup_entry_for_deref(struct copy_prop_var_state *state,<br>
> +                       nir_<wbr>deref_var *deref,<br>
> +                       enum deref_compare_result<br>
> allowed_comparisons)<br>
> +{<br>
> +   list_for_each_entry(struct copy_entry, iter, &state->copies,<br>
> link) {<br>
> +      if (compare_derefs(iter->dst, deref) & allowed_comparisons)<br>
> +         return iter;<br>
> +   }<br>
> +<br>
> +   return NULL;<br>
> +}<br>
> +<br>
> +static void<br>
> +mark_aliased_entries_as_read(<wbr>struct copy_prop_var_state *state,<br>
> +                             <wbr>nir_deref_var *deref, unsigned<br>
> components)<br>
> +{<br>
> +   list_for_each_entry(struct copy_entry, iter, &state->copies,<br>
> link) {<br>
> +      if (compare_derefs(iter->dst, deref) & derefs_may_alias_bit)<br>
> +         iter->comps_may_be_<wbr>read |= components;<br>
> +   }<br>
> +}<br>
> +<br>
> +static struct copy_entry *<br>
> +get_entry_and_kill_aliases(<wbr>struct copy_prop_var_state *state,<br>
> +                           <wbr>nir_deref_var *deref,<br>
> +                           <wbr>unsigned write_mask)<br>
> +{<br>
> +   struct copy_entry *entry = NULL;<br>
> +   list_for_each_entry_safe(<wbr>struct copy_entry, iter, &state->copies,<br>
> link) {<br>
> +      if (!iter->src.is_ssa) {<br>
> +         /* If this write aliases the source of some entry, get rid<br>
> of it */<br>
> +         if (compare_derefs(iter->src.<wbr>deref, deref) &<br>
> derefs_may_alias_bit) {<br>
> +            copy_entry_<wbr>destroy(state, iter);<br>
> +            continue;<br>
> +         }<br>
> +      }<br>
> +<br>
> +      enum deref_compare_result comp = compare_derefs(iter->dst,<br>
> deref);<br>
> +      /* This is a store operation.  If we completely overwrite some<br>
> value, we<br>
> +       * want to delete any dead writes that may be present.<br>
> +       */<br>
> +      if (comp & derefs_b_contains_a_bit)<br>
> +         remove_dead_writes(<wbr>state, iter, write_mask);<br>
> +<br>
> +      if (comp & derefs_equal_bit) {<br>
> +         assert(entry == NULL);<br>
> +         entry = iter;<br>
> +      } else if (comp & derefs_may_alias_bit) {<br>
> +         copy_entry_destroy(<wbr>state, iter);<br>
> +      }<br>
> +   }<br>
> +<br>
> +   if (entry == NULL)<br>
> +      entry = copy_entry_create(state, deref);<br>
> +<br>
> +   return entry;<br>
> +}<br>
> +<br>
> +static void<br>
> +apply_barrier_for_modes(<wbr>struct copy_prop_var_state *state,<br>
> +                        nir_<wbr>variable_mode modes)<br>
> +{<br>
> +   list_for_each_entry_safe(<wbr>struct copy_entry, iter, &state->copies,<br>
> link) {<br>
> +      if ((iter->dst->var->data.mode & modes) ||<br>
> +          (!(iter->src.is_<wbr>ssa) && (iter->src.deref->var->data.<wbr>mode &<br>
> modes)))<br>
<br>
</div></div> !(iter->src.is_ssa) -> !iter->src.is_ssa ??<br></blockquote><div><br></div><div>Sure.  Changed locally.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5">
> +         copy_entry_destroy(<wbr>state, iter);<br>
> +   }<br>
> +}<br>
> +<br>
> +static void<br>
> +store_to_entry(struct copy_prop_var_state *state, struct copy_entry<br>
> *entry,<br>
> +               const struct value *value, unsigned write_mask,<br>
> +               nir_instr *store_instr)<br>
> +{<br>
> +   entry->comps_may_be_read &= ~write_mask;<br>
> +   if (value->is_ssa) {<br>
> +      entry->src.is_ssa = true;<br>
> +      /* Only overwrite the written components */<br>
> +      for (unsigned i = 0; i < 4; i++) {<br>
> +         if (write_mask & (1 << i)) {<br>
> +            entry->store_<wbr>instr[i] = store_instr;<br>
> +            entry->src.ssa[i] = value->ssa[i];<br>
> +         }<br>
> +      }<br>
> +   } else {<br>
> +      /* Non-ssa stores always write everything */<br>
> +      entry->src.is_ssa = false;<br>
> +      entry->src.deref = value->deref;<br>
> +      for (unsigned i = 0; i < 4; i++)<br>
> +         entry->store_instr[<wbr>i] = store_instr;<br>
> +   }<br>
> +}<br>
> +<br>
> +/* Remove an instruction and return a cursor pointing to where it<br>
> was */<br>
> +static nir_cursor<br>
> +instr_remove_cursor(nir_instr *instr)<br>
> +{<br>
> +   nir_cursor cursor;<br>
> +   nir_instr *prev = nir_instr_prev(instr);<br>
> +   if (prev) {<br>
> +      cursor = nir_after_instr(prev);<br>
> +   } else {<br>
> +      cursor = nir_before_block(instr->block)<wbr>;<br>
> +   }<br>
> +   nir_instr_remove(instr);<br>
> +   return cursor;<br>
> +}<br>
<br>
</div></div>I wonder if this is useful elsewhere?<br><div><div class="gmail-h5"></div></div></blockquote><div><br></div><div>I wondered that too.  I'm happy to move it to nir.c if you'd like.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5">
> +<br>
> +static bool<br>
> +load_from_ssa_entry_value(<wbr>struct copy_prop_var_state *state,<br>
> +                          <wbr>struct copy_entry *entry,<br>
> +                          <wbr>nir_builder *b, nir_intrinsic_instr<br>
> *intrin,<br>
> +                          <wbr>struct value *value)<br>
> +{<br>
> +   *value = entry->src;<br>
> +   assert(value->is_ssa);<br>
> +<br>
> +   const struct glsl_type *type = nir_deref_tail(&entry->dst-<br>
> >deref)->type;<br>
> +   unsigned num_components = glsl_get_vector_elements(type)<wbr>;<br>
> +<br>
> +   uint8_t available = 0;<br>
> +   bool all_same = true;<br>
> +   for (unsigned i = 0; i < num_components; i++) {<br>
> +      if (value->ssa[i])<br>
> +         available |= (1 << i);<br>
> +<br>
> +      if (value->ssa[i] != value->ssa[0])<br>
> +         all_same = false;<br>
> +   }<br>
> +<br>
> +   if (all_same) {<br>
> +      /* Our work here is done */<br>
<br>
</div></div>Mind expanding on this? :P Without looking at the caller I have no idea<br>
 why the instruction is being removed. Going on the function name I<br>
suspect its because we can use the ssa entry value instead since it<br>
contains all the values we are looking for but it would be nice to be<br>
clear.<br>
<br>
The other alternative is to add a general description to the function<br>
itself.<br><div><div class="gmail-h5"></div></div></blockquote><div><br></div>Right... I can add a nice detailed comment above the function.<br></div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5">
> +      b->cursor = instr_remove_cursor(&intrin-><wbr>instr);<br>
> +      intrin->instr.block = NULL;<br>
> +      return true;<br>
> +   }<br>
> +<br>
> +   if (available != (1 << num_components) - 1 &&<br>
> +       intrin->intrinsic == nir_intrinsic_load_var &&<br>
> +       (available & nir_ssa_def_components_read(&<wbr>intrin->dest.ssa))<br>
> == 0) {<br>
> +      /* If none of the components read are available as SSA values,<br>
> then we<br>
> +       * should just bail.  Otherwise, we would end up replacing the<br>
> uses of<br>
> +       * the load_var a vecN() that just gathers up its components.<br>
> +       */<br>
> +      return false;<br>
> +   }<br>
> +<br>
> +   b->cursor = nir_after_instr(&intrin-><wbr>instr);<br>
> +<br>
> +   nir_ssa_def *load_def =<br>
> +      intrin->intrinsic == nir_intrinsic_load_var ? &intrin-<br>
> >dest.ssa : NULL;<br>
> +<br>
> +   bool keep_intrin = false;<br>
> +   nir_ssa_def *comps[4];<br>
> +   for (unsigned i = 0; i < num_components; i++) {<br>
> +      if (value->ssa[i]) {<br>
> +         comps[i] = nir_channel(b, value->ssa[i], i);<br>
> +      } else {<br>
> +         /* We don't have anything for this component in our<br>
> +          * list.  Just re-use a channel from the load.<br>
> +          */<br>
> +         if (load_def == NULL)<br>
> +            load_def = nir_load_deref_var(b, entry->dst);<br>
> +<br>
> +         if (load_def->parent_instr == &intrin->instr)<br>
> +            keep_intrin = true;<br>
> +<br>
> +         comps[i] = nir_channel(b, load_def, i);<br>
> +      }<br>
> +   }<br>
> +<br>
> +   nir_ssa_def *vec = nir_vec(b, comps, num_components);<br>
> +   for (unsigned i = 0; i < num_components; i++)<br>
> +      value->ssa[i] = vec;<br>
> +<br>
> +   if (!keep_intrin) {<br>
> +      /* Removing this instruction should not touch the cursor<br>
> because we<br>
> +       * created the cursor after the intrinsic and have added at<br>
> least one<br>
> +       * instruction (the vec) since then.<br>
> +       */<br>
<br>
</div></div>I might be blind but where was that added? Do we need an assert here?<br></blockquote><div><br></div><div>right above the for above this if.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
assert(cursor != intrin->instr)<br></blockquote><div><br></div><div>Sure<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div class="gmail-h5"><br>
> +      nir_instr_remove(&<wbr>intrin->instr);<br>
> +      intrin->instr.block = NULL;<br>
> +   }<br>
> +<br>
> +   return true;<br>
> +}<br>
> +<br>
> +/**<br>
> + * Specialize the wildcards in a deref chain<br>
> + *<br>
> + * This function returns a deref chain identical to \param deref<br>
> except that<br>
> + * some of its wildcards are replaced with indices from \param<br>
> specific.  The<br>
> + * process is guided by \param guide which references the same type<br>
> as \param<br>
> + * specific but has the same wildcard array lengths as \param deref.<br>
> + */<br>
> +static nir_deref_var *<br>
> +specialize_wildcards(nir_<wbr>deref_var *deref,<br>
> +                     nir_<wbr>deref_var *guide,<br>
> +                     nir_<wbr>deref_var *specific,<br>
> +                     void *mem_ctx)<br>
> +{<br>
> +   nir_deref_var *ret = nir_deref_var_create(mem_ctx, deref->var);<br>
> +<br>
> +   nir_deref *deref_tail = deref->deref.child;<br>
> +   nir_deref *guide_tail = guide->deref.child;<br>
> +   nir_deref *spec_tail = specific->deref.child;<br>
> +   nir_deref *ret_tail = &ret->deref;<br>
> +   while (deref_tail) {<br>
> +      switch (deref_tail->deref_type) {<br>
> +      case nir_deref_type_array: {<br>
> +         nir_deref_array *deref_arr =<br>
> nir_deref_as_array(deref_tail)<wbr>;<br>
> +<br>
> +         nir_deref_array *ret_arr =<br>
> nir_deref_array_create(ret_<wbr>tail);<br>
> +         ret_arr->deref.type = deref_arr->deref.type;<br>
> +         ret_arr->deref_<wbr>array_type = deref_arr->deref_array_type;<br>
> +<br>
> +         switch (deref_arr->deref_array_type) {<br>
> +         case nir_deref_array_type_direct:<br>
> +            ret_arr->base_<wbr>offset = deref_arr->base_offset;<br>
> +            break;<br>
> +         case nir_deref_array_type_indirect:<br>
> +            ret_arr->base_<wbr>offset = deref_arr->base_offset;<br>
> +            assert(deref_arr-<wbr>>indirect.is_ssa);<br>
> +            ret_arr->indirect = deref_arr->indirect;<br>
> +            break;<br>
> +         case nir_deref_array_type_wildcard:<br>
> +            /* This is where things get tricky.  We have to search<br>
> through<br>
> +             * the entry deref to find its corresponding wildcard<br>
> and fill<br>
> +             * this slot in with the value from the src.<br>
> +             */<br>
> +            while (guide_tail) {<br>
> +               if (guide_tail->deref_type == nir_deref_type_array &&<br>
> +                   nir_deref_<wbr>as_array(guide_tail)->deref_<wbr>array_type<br>
> ==<br>
> +                   nir_deref_<wbr>array_type_wildcard)<br>
> +                  break;<br>
> +<br>
> +               guide_tail = guide_tail->child;<br>
> +               spec_tail = spec_tail->child;<br>
> +            }<br>
> +<br>
> +            nir_deref_array *spec_arr =<br>
> nir_deref_as_array(spec_tail);<br>
> +            ret_arr->deref_<wbr>array_type = spec_arr->deref_array_type;<br>
> +            ret_arr->base_<wbr>offset = spec_arr->base_offset;<br>
> +            ret_arr->indirect = spec_arr->indirect;<br>
> +         }<br>
> +<br>
> +         ret_tail->child = &ret_arr->deref;<br>
> +         break;<br>
> +      }<br>
> +      case nir_deref_type_struct: {<br>
> +         nir_deref_struct *deref_struct =<br>
> nir_deref_as_struct(deref_<wbr>tail);<br>
> +<br>
> +         nir_deref_struct *ret_struct =<br>
> +            nir_deref_struct_<wbr>create(ret_tail, deref_struct->index);<br>
> +         ret_struct->deref.<wbr>type = deref_struct->deref.type;<br>
> +<br>
> +         ret_tail->child = &ret_struct->deref;<br>
> +         break;<br>
> +      }<br>
> +      case nir_deref_type_var:<br>
> +         unreachable("Invalid deref type");<br>
> +      }<br>
> +<br>
> +      deref_tail = deref_tail->child;<br>
> +      ret_tail = ret_tail->child;<br>
> +   }<br>
> +<br>
> +   return ret;<br>
> +}<br>
> +<br>
> +static bool<br>
> +load_from_deref_entry_value(<wbr>struct copy_prop_var_state *state,<br>
> +                            <wbr>struct copy_entry *entry,<br>
> +                            <wbr>nir_builder *b, nir_intrinsic_instr<br>
> *intrin,<br>
> +                            <wbr>nir_deref_var *src, struct value *value)<br>
> +{<br>
> +   *value = entry->src;<br>
> +<br>
> +   /* Walk the deref to get the two tails and also figure out if we<br>
> need to<br>
> +    * specialize any wildcards.<br>
> +    */<br>
> +   bool need_to_specialize_wildcards = false;<br>
> +   nir_deref *entry_tail = &entry->dst->deref;<br>
> +   nir_deref *src_tail = &src->deref;<br>
> +   while (entry_tail->child && src_tail->child) {<br>
> +      assert(src_tail->child-<wbr>>deref_type == entry_tail->child-<br>
> >deref_type);<br>
> +      if (src_tail->child->deref_type == nir_deref_type_array) {<br>
> +         nir_deref_array *entry_arr = nir_deref_as_array(entry_tail-<br>
> >child);<br>
> +         nir_deref_array *src_arr = nir_deref_as_array(src_tail-<br>
> >child);<br>
> +<br>
> +         if (src_arr->deref_array_type !=<br>
> nir_deref_array_type_wildcard &&<br>
> +             entry_arr-><wbr>deref_array_type ==<br>
> nir_deref_array_type_wildcard)<br>
> +            need_to_<wbr>specialize_wildcards = true;<br>
> +      }<br>
> +<br>
> +      entry_tail = entry_tail->child;<br>
> +      src_tail = src_tail->child;<br>
> +   }<br>
> +<br>
> +   /* If the entry deref is longer than the source deref then it<br>
> refers to a<br>
> +    * smaller type and we can't source from it.<br>
> +    */<br>
> +   assert(entry_tail->child == NULL);<br>
> +<br>
> +   if (need_to_specialize_wildcards) {<br>
> +      /* The entry has some wildcards that are not in src.  This<br>
> means we need<br>
> +       * to construct a new deref based on the entry but using the<br>
> wildcards<br>
> +       * from the source and guided by the entry dst.  Oof.<br>
> +       */<br>
> +      value->deref = specialize_wildcards(entry-><wbr>src.deref, entry-<br>
> >dst, src,<br>
> +                             <wbr>             state->mem_ctx);<br>
> +   } else {<br>
> +      /* We're going to need to make a copy anyway... */<br>
<br>
</div></div>because? If its worth commenting on you might as well finish the<br>
comment. Otherwise if you think its obvious can you just remove it. To<br>
be honest I'm not sure what is going on here exactly so it would be<br>
good if you could expand the comment to make it totally obvious.<span class="gmail-"><br></span></blockquote><div><br></div><div>I added the following comment:<br><br>/* Do a "load" from an deref-based entry return it in "value" as a value.  The<br> * deref returned in "value" will always be a fresh copy so the caller can<br> * steal it and assign it to the instruction directly without copying it<br> * again.<br> */<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">
> +      value->deref =<br>
> +         nir_deref_as_var(<wbr>nir_copy_deref(state->mem_ctx, &value-<br>
> >deref->deref));<br>
> +   }<br>
> +<br>
> +   if (src_tail->child) {<br>
> +      /* If our source deref is longer than the entry deref, that's<br>
> ok because<br>
> +       * it just means the entry deref needs to be extended a bit.<br>
> +       */<br>
> +      nir_deref *value_tail = nir_deref_tail(&value->deref-><wbr>deref);<br>
> +      value_tail->child = nir_copy_deref(value_tail, src_tail-<br>
> >child);<br>
<br>
</span>So we need to copy above because we might modify things here?<br></blockquote><div><br></div><div>Also, yes, we need a copy so that we can modify it.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Ok returning here after reviewing below it seems we steal the deref so<br>
I guess that's why we want a copy.<br>
<div><div class="gmail-h5"><br>
> +   }<br>
> +<br>
> +   b->cursor = instr_remove_cursor(&intrin-><wbr>instr);<br>
> +<br>
> +   return true;<br>
> +}<br>
> +<br>
> +static bool<br>
> +try_load_from_entry(struct copy_prop_var_state *state, struct<br>
> copy_entry *entry,<br>
> +                    nir_<wbr>builder *b, nir_intrinsic_instr *intrin,<br>
> +                    nir_<wbr>deref_var *src, struct value *value)<br>
> +{<br>
> +   if (entry == NULL)<br>
> +      return false;<br>
> +<br>
> +   if (entry->src.is_ssa) {<br>
> +      return load_from_ssa_entry_value(<wbr>state, entry, b, intrin,<br>
> value);<br>
> +   } else {<br>
> +      return load_from_deref_entry_value(<wbr>state, entry, b, intrin,<br>
> src, value);<br>
> +   }<br>
> +}<br>
> +<br>
> +static void<br>
> +copy_prop_vars_block(struct copy_prop_var_state *state,<br>
> +                     nir_<wbr>builder *b, nir_block *block)<br>
> +{<br>
> +   /* Start each block with a blank slate */<br>
> +   list_for_each_entry_safe(<wbr>struct copy_entry, iter, &state->copies,<br>
> link)<br>
> +      copy_entry_destroy(<wbr>state, iter);<br>
> +<br>
> +   nir_foreach_instr_safe(<wbr>instr, block) {<br>
> +      if (instr->type != nir_instr_type_intrinsic)<br>
> +         continue;<br>
> +<br>
> +      nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);<br>
> +      switch (intrin->intrinsic) {<br>
> +      case nir_intrinsic_barrier:<br>
> +      case nir_intrinsic_memory_barrier:<br>
> +         /* If we hit a barrier, we need to trash everything that<br>
> may possible<br>
<br>
</div></div>may possibly<span class="gmail-"><br></span></blockquote><div><br></div><div>yup<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">
> +          * be accessible to another thread.  Locals, globals, and<br>
> things of<br>
> +          * the like are safe, however.<br>
> +          */<br>
> +         apply_barrier_for_<wbr>modes(state, ~(nir_var_local |<br>
> nir_var_global |<br>
> +                             <wbr>             nir_var_shader_in |<br>
> nir_var_uniform));<br>
> +         break;<br>
> +<br>
> +      case nir_intrinsic_emit_vertex:<br>
> +      case nir_intrinsic_emit_vertex_<wbr>with_counter:<br>
> +         apply_barrier_for_<wbr>modes(state, nir_var_shader_out);<br>
> +         break;<br>
> +<br>
> +      case nir_intrinsic_load_var: {<br>
<br>
</span>Is it worth short circuiting this?<br>
<br>
  if (list_empty(&state->copies))<br>
     continue;<br>
<br>
Looking closely below I'm not sure it is.<br>
<br>
Never mind I see we need to create the new copy entry below regardless.<br>
<div><div class="gmail-h5"><br>
> +         nir_deref_var *src = intrin->variables[0];<br>
> +<br>
> +         uint8_t comps_read = nir_ssa_def_components_read(&<wbr>intrin-<br>
> >dest.ssa);<br>
> +         mark_aliased_<wbr>entries_as_read(state, src, comps_read);<br>
> +<br>
> +         struct copy_entry *src_entry =<br>
> +            lookup_entry_for_<wbr>deref(state, src,<br>
> derefs_a_contains_b_bit);<br>
> +         struct value value;<br>
> +         if (try_load_from_entry(state, src_entry, b, intrin, src,<br>
> &value)) {<br>
> +            if (value.is_ssa) {<br>
> +               /* lookup_load has already ensured that we get a<br>
> single SSA<br>
> +                * value that has all of the channels.  We just have<br>
> to do the<br>
> +                * rewrite operation.<br>
> +                */<br>
> +               if (intrin->instr.block) {<br>
> +                  /* The lookup left our instruction in-place.  This<br>
> means it<br>
> +                   * must have used it to vec up a bunch of<br>
> different sources.<br>
> +                   * We need to be careful<br>
<br>
</div></div>Is this the end of the sentence or is there meant to be more here?<br></blockquote><div><br></div><div>I finished the sentence.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5">
> +                   */<br>
> +                  nir_ssa_<wbr>def_rewrite_uses_after(&<wbr>intrin->dest.ssa,<br>
> +                             <wbr>                    nir_src_<wbr>for_ssa(val<br>
> ue.ssa[0]),<br>
> +                             <wbr>                    value.ssa[<wbr>0]-<br>
> >parent_instr);<br>
> +               } else {<br>
> +                  nir_ssa_<wbr>def_rewrite_uses(&intrin-><wbr>dest.ssa,<br>
> +                             <wbr>              nir_src_for_ssa(<wbr>value.ssa<br>
> [0]));<br>
> +               }<br>
> +            } else {<br>
> +               /* We're turning it into a load of a different<br>
> variable */<br>
> +               ralloc_steal(<wbr>intrin, value.deref);<br>
> +               intrin-><wbr>variables[0] = value.deref;<br>
> +<br>
> +               /* Put it back in again. */<br>
> +               nir_builder_<wbr>instr_insert(b, instr);<br>
> +<br>
> +               value.is_ssa = true;<br>
> +               for (unsigned i = 0; i < intrin->num_components; i++)<br>
> +                  value.ssa[<wbr>i] = &intrin->dest.ssa;<br>
> +            }<br>
> +            state->progress = true;<br>
> +         } else {<br>
> +            value.is_ssa = true;<br>
> +            for (unsigned i = 0; i < intrin->num_components; i++)<br>
> +               value.ssa[i] = &intrin->dest.ssa;<br>
> +         }<br>
> +<br>
> +         /* Now that we have a value, we're going to store it back<br>
> so that we<br>
> +          * have the right value next time we come looking for<br>
> it.  In order<br>
> +          * to do this, we need an exact match, not just something<br>
> that<br>
> +          * contains what we're looking for.<br>
> +          */<br>
> +         struct copy_entry *store_entry =<br>
> +            lookup_entry_for_<wbr>deref(state, src, derefs_equal_bit);<br>
> +         if (!store_entry)<br>
> +            store_entry = copy_entry_create(state, src);<br>
</div></div>> +store_to_entry<br>
<div class="gmail-HOEnZb"><div class="gmail-h5">> +         /* Set up a store to this entry with the value of the<br>
> load.  This way<br>
> +          * we can potentially remove subsequent loads.  However, we<br>
> use a<br>
> +          * NULL instruction so we don't try and delete the load on<br>
> a<br>
> +          * subsequent store.<br>
> +          */<br>
> +         store_to_entry(<wbr>state, store_entry, &value,<br>
> +                        ((1 << intrin->num_components) - 1), NULL);<br>
> +         break;<br>
> +      }<br>
> +<br>
> +      case nir_intrinsic_store_var: {<br>
> +         struct value value = {<br>
> +            .is_ssa = true<br>
> +         };<br>
> +<br>
> +         for (unsigned i = 0; i < intrin->num_components; i++)<br>
> +            value.ssa[i] = intrin->src[0].ssa;<br>
> +<br>
> +         nir_deref_var *dst = intrin->variables[0];<br>
> +         unsigned wrmask = nir_intrinsic_write_mask(<wbr>intrin);<br>
> +         struct copy_entry *entry =<br>
> +            get_entry_and_<wbr>kill_aliases(state, dst, wrmask);<br>
> +         store_to_entry(<wbr>state, entry, &value, wrmask, &intrin-<br>
> >instr);<br>
> +         break;<br>
> +      }<br>
> +<br>
> +      case nir_intrinsic_copy_var: {<br>
> +         nir_deref_var *dst = intrin->variables[0];<br>
> +         nir_deref_var *src = intrin->variables[1];<br>
> +<br>
> +         if (compare_derefs(src, dst) & derefs_equal_bit) {<br>
> +            /* This is a no-op self-copy.  Get rid of it */<br>
> +            nir_instr_remove(<wbr>instr);<br>
> +            continue;<br>
> +         }<br>
> +<br>
> +         mark_aliased_<wbr>entries_as_read(state, src, 0xf);<br>
> +<br>
> +         struct copy_entry *src_entry =<br>
> +            lookup_entry_for_<wbr>deref(state, src,<br>
> derefs_a_contains_b_bit);<br>
> +         struct value value;<br>
> +         if (try_load_from_entry(state, src_entry, b, intrin, src,<br>
> &value)) {<br>
> +            if (value.is_ssa) {<br>
> +               nir_store_<wbr>deref_var(b, dst, value.ssa[0], 0xf);<br>
> +               intrin =<br>
> nir_instr_as_intrinsic(nir_<wbr>builder_last_instr(b));<br>
> +            } else {<br>
> +               /* If this would be a no-op self-copy, don't bother.<br>
> */<br>
> +               if (compare_derefs(value.deref, dst) &<br>
> derefs_equal_bit)<br>
> +                  continue;<br>
> +<br>
> +               /* Just turn it into a copy of a different deref */<br>
> +               ralloc_steal(<wbr>intrin, value.deref);<br>
> +               intrin-><wbr>variables[1] = value.deref;<br>
> +<br>
> +               /* Put it back in again. */<br>
> +               nir_builder_<wbr>instr_insert(b, instr);<br>
> +            }<br>
> +<br>
> +            state->progress = true;<br>
> +         } else {<br>
> +            value = (struct value) {<br>
> +               .is_ssa = false,<br>
> +               .deref = src,<br>
> +            };<br>
> +         }<br>
> +<br>
> +         struct copy_entry *dst_entry =<br>
> +            get_entry_and_<wbr>kill_aliases(state, dst, 0xf);<br>
> +         store_to_entry(<wbr>state, dst_entry, &value, 0xf, &intrin-<br>
> >instr);<br>
> +         break;<br>
> +      }<br>
> +<br>
> +      default:<br>
> +         break;<br>
> +      }<br>
> +   }<br>
> +}<br>
> +<br>
> +bool<br>
> +nir_opt_copy_prop_vars(nir_<wbr>shader *shader)<br>
> +{<br>
> +   struct copy_prop_var_state state;<br>
> +<br>
> +   state.shader = shader;<br>
> +   state.mem_ctx = ralloc_context(NULL);<br>
> +   list_inithead(&state.<wbr>copies);<br>
> +   list_inithead(&state.copy_<wbr>free_list);<br>
> +<br>
> +   bool global_progress = false;<br>
> +   nir_foreach_function(<wbr>function, shader) {<br>
> +      if (!function->impl)<br>
> +         continue;<br>
> +<br>
> +      nir_builder b;<br>
> +      nir_builder_init(&b, function->impl);<br>
> +<br>
> +      state.progress = false;<br>
> +      nir_foreach_block(<wbr>block, function->impl)<br>
> +         copy_prop_vars_<wbr>block(&state, &b, block);<br>
> +<br>
> +      if (state.progress) {<br>
> +         nir_metadata_<wbr>preserve(function->impl,<br>
> nir_metadata_block_index |<br>
> +                             <wbr>                  nir_<wbr>metadata_dominanc<br>
> e);<br>
> +         global_progress = true;<br>
> +      }<br>
> +   }<br>
> +<br>
> +   ralloc_free(state.mem_ctx)<wbr>;<br>
> +<br>
> +   return global_progress;<br>
> +}<br>
</div></div></blockquote></div><br></div></div>