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