<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Aug 21, 2018 at 7:49 PM Caio Marcelo de Oliveira Filho <<a href="mailto:caio.oliveira@intel.com">caio.oliveira@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Sat, Jul 28, 2018 at 10:44:41PM -0700, Jason Ekstrand wrote:<br>
> This peephole optimization looks for a series of load/store_deref or<br>
> copy_deref instructions that copy an array from one variable to another<br>
> and turns it into a copy_deref that copies the entire array. The<br>
> pattern it looks for is extremely specific but it's good enough to pick<br>
> up on the input array copies in DXVK and should also be able to pick up<br>
> the sequence generated by spirv_to_nir for a OpLoad of a large composite<br>
> followed by OpStore. It can always be improved later if needed.<br>
<br>
I've liked this one :-)<br>
<br>
I'd add a comment to the file with a subset of this description. In<br>
particular I'd consider mentioning that the pass expects the<br>
loads/stores/copies to happen in order and also any other<br>
stores/copies to different variables will spoil the detecting.<br>
<br>
There are a few minor comments and a comment about store write mask.<br>
Assuming those are resolved this patch is:<br>
<br>
Reviewed-by: Caio Marcelo de Oliveira Filho <<a href="mailto:caio.oliveira@intel.com" target="_blank">caio.oliveira@intel.com</a>><br></blockquote><div><br></div><div>I've applied your suggestions. Please double-check:</div><div><br></div><div><a href="https://gitlab.freedesktop.org/jekstrand/mesa/commit/28c01e9a0ce84ff53fa8805e4a35a691ea3fc744">https://gitlab.freedesktop.org/jekstrand/mesa/commit/28c01e9a0ce84ff53fa8805e4a35a691ea3fc744</a><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">
Note: the "deref_map" (well, an evolution of it) from the dead write<br>
elimination series, could be used to keep track of multiple of such<br>
copies, with less restrictions. That said, seems for the case we<br>
about it is not necessary.<br>
<br>
(...)<br>
<br>
> +static bool<br>
> +opt_find_array_copies_block(nir_builder *b, nir_block *block,<br>
> + unsigned *num_ssa_defs, void *mem_ctx)<br>
> +{<br>
> + bool progress = false;<br>
> +<br>
> + struct match_state s;<br>
> + match_state_init(&s);<br>
> +<br>
> + nir_variable *dst_var = NULL;<br>
> + unsigned prev_dst_var_last_write = *num_ssa_defs;<br>
> + unsigned dst_var_last_write = *num_ssa_defs;<br>
> +<br>
> + nir_foreach_instr(instr, block) {<br>
> + /* Index the SSA defs before we do anything else. */<br>
> + nir_foreach_ssa_def(instr, index_ssa_def_cb, num_ssa_defs);<br>
> +<br>
> + if (instr->type != nir_instr_type_intrinsic)<br>
> + continue;<br>
> +<br>
> + nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);<br>
> + if (intrin->intrinsic != nir_intrinsic_copy_deref &&<br>
> + intrin->intrinsic != nir_intrinsic_store_deref)<br>
> + continue;<br>
> +<br>
> + nir_deref_instr *dst_deref = nir_src_as_deref(intrin->src[0]);<br>
<br>
Given it is cheap enough, would make sense to early return if<br>
dst_deref is a var, at least before we make it an active copy?<br>
Similar case for src_deref. The code is correct (try_match_deref will<br>
fail for variable since it can find an index), so let this up to you.<br></blockquote><div><br></div><div>I think we probably could. I'll add something.<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>
> + /* The destination must be local. If we see a non-local store, we<br>
> + * continue on because it won't affect local stores or read-only<br>
> + * variables.<br>
> + */<br>
> + if (dst_deref->mode != nir_var_local)<br>
> + continue;<br>
<br>
For correctness, should we reset if this is not a full write mask<br>
store?<br></blockquote><div><br></div><div>Yes, we should. I'm not sure if that can happen in practice right now but we should put it in there.</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">
> + /* We keep track of the SSA indicies where the two last-written<br>
<br>
Typo here and elsewhere: "indices".<br></blockquote><div><br></div><div>Fixed. Thanks!<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">
> + * variables are written. The prev_dst_var_last_write tells us when<br>
> + * the last store_deref to something other than dst happened. If the<br>
> + * SSA def index from a load is greater than or equal to this number<br>
> + * then we know it happened afterwards and no writes to anything other<br>
> + * than dst occur between the load and the current instruction.<br>
> + */<br>
> + if (nir_deref_instr_get_variable(dst_deref) != dst_var) {<br>
> + prev_dst_var_last_write = dst_var_last_write;<br>
> + dst_var = nir_deref_instr_get_variable(dst_deref);<br>
> + }<br>
> + dst_var_last_write = *num_ssa_defs;<br>
> +<br>
> + nir_deref_instr *src_deref;<br>
> + if (intrin->intrinsic == nir_intrinsic_copy_deref) {<br>
> + src_deref = nir_src_as_deref(intrin->src[1]);<br>
> + } else {<br>
> + assert(intrin->intrinsic == nir_intrinsic_store_deref);<br>
> + src_deref = get_deref_for_load_src(intrin->src[1],<br>
> + prev_dst_var_last_write);<br>
> + }<br>
> +<br>
> + /* If we didn't find a valid src, then we have an unknown store and it<br>
> + * could mess things up.<br>
> + */<br>
> + if (src_deref == NULL)<br>
> + goto fail;<br>
> +<br>
> + /* The destination must be local and the source must be either local or<br>
> + * something that's guaranteed to be read-only.<br>
> + */<br>
> + const nir_variable_mode read_only_modes =<br>
> + nir_var_shader_in | nir_var_uniform | nir_var_system_value;<br>
> + if (!(src_deref->mode & (nir_var_local | read_only_modes)))<br>
> + goto fail;<br>
> +<br>
> + /* If we don't yet have an active copy, then make this instruction the<br>
> + * active copy.<br>
> + */<br>
<br>
A note explaining that in the first store we still don't know which<br>
array in the path is being copied would have helped me when reading<br>
this.<br></blockquote><div><br></div><div>Done.<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">
> + if (s.next_array_idx == 0) {<br>
> + /* We can't combine a copy if there is any chance the source and<br>
> + * destination will end up aliasing. Just bail if they're the same<br>
> + * variable.<br>
> + */<br>
> + if (nir_deref_instr_get_variable(dst_deref) ==<br>
> + nir_deref_instr_get_variable(src_deref))<br>
<br>
Could reuse dst_var instead of recalculate.<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">
> + goto fail;<br>
> +<br>
> + /* The load/store pair is enough to guarantee the same bit size and<br>
> + * number of components but a copy_var requires the actual types to<br>
> + * match.<br>
> + */<br>
> + if (dst_deref->type != src_deref->type)<br>
> + continue;<br>
> +<br>
> + nir_deref_path_init(&s.first_dst_path, dst_deref, mem_ctx);<br>
> + nir_deref_path_init(&s.first_src_path, src_deref, mem_ctx);<br>
> + s.next_array_idx = 1;<br>
> + continue;<br>
> + }<br>
> +<br>
> + if (!try_match_deref(&s.first_dst_path, &s.dst_deref_array_idx,<br>
> + dst_deref, s.next_array_idx, mem_ctx) ||<br>
> + !try_match_deref(&s.first_src_path, &s.src_deref_array_idx,<br>
> + src_deref, s.next_array_idx, mem_ctx))<br>
> + goto fail;<br>
> +<br>
> + if (s.next_array_idx == 1) {<br>
> + /* This is our first non-trivial match. We now have indicies into<br>
> + * the search paths so we can do a couple more checks.<br>
> + */<br>
> + assert(s.dst_deref_array_idx > 0 && s.src_deref_array_idx > 0);<br>
> + const struct glsl_type *dst_arr_type =<br>
> + s.first_dst_path.path[s.dst_deref_array_idx - 1]->type;<br>
> + const struct glsl_type *src_arr_type =<br>
> + s.first_src_path.path[s.src_deref_array_idx - 1]->type;<br>
> +<br>
> + assert(glsl_type_is_array(dst_arr_type) ||<br>
> + glsl_type_is_matrix(dst_arr_type));<br>
> + assert(glsl_type_is_array(src_arr_type) ||<br>
> + glsl_type_is_matrix(src_arr_type));<br>
> +<br>
> + /* They must be the same length */<br>
> + s.array_len = glsl_get_length(dst_arr_type);<br>
> + if (s.array_len != glsl_get_length(src_arr_type))<br>
> + goto fail;<br>
> + }<br>
> +<br>
> + s.next_array_idx++;<br>
> +<br>
> + if (s.next_array_idx == s.array_len) {<br>
> + /* Hooray, We found a copy! */<br>
> + b->cursor = nir_after_instr(instr);<br>
> + nir_copy_deref(b, build_wildcard_deref(b, &s.first_dst_path,<br>
> + s.dst_deref_array_idx),<br>
> + build_wildcard_deref(b, &s.first_src_path,<br>
> + s.src_deref_array_idx));<br>
> + match_state_reset(&s);<br>
> + progress = true;<br>
> + }<br>
> +<br>
> + continue;<br>
> +<br>
> + fail:<br>
> + match_state_reset(&s);<br>
> + }<br>
<br>
Maybe call this label "reset:" instead of "fail:".<br></blockquote><div><br></div><div>Sure. That's reasonable.</div><div><br></div><div>--Jason<br></div></div></div>