<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 26, 2015 at 11:21 AM, Eric Anholt <span dir="ltr"><<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes:<br>
> diff --git a/src/glsl/nir/nir_lower_phis_to_scalar.c b/src/glsl/nir/nir_lower_phis_to_scalar.c<br>
> new file mode 100644<br>
> index 0000000..9f901d6<br>
> --- /dev/null<br>
> +++ b/src/glsl/nir/nir_lower_phis_to_scalar.c<br>
> @@ -0,0 +1,238 @@<br>
> +/*<br>
> + * Copyright © 2014 Intel Corporation<br>
<br>
</span>Possibly update your copyright date?<br></blockquote><div><br></div><div>Fixed.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> + *<br>
> + * Permission is hereby granted, free of charge, to any person obtaining a<br>
> + * copy of this software and associated documentation files (the "Software"),<br>
> + * to deal in the Software without restriction, including without limitation<br>
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,<br>
> + * and/or sell copies of the Software, and to permit persons to whom the<br>
> + * Software is furnished to do so, subject to the following conditions:<br>
> + *<br>
> + * The above copyright notice and this permission notice (including the next<br>
> + * paragraph) shall be included in all copies or substantial portions of the<br>
> + * Software.<br>
> + *<br>
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR<br>
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,<br>
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL<br>
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER<br>
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING<br>
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS<br>
> + * IN THE SOFTWARE.<br>
> + *<br>
> + * Authors:<br>
> + * Jason Ekstrand (<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>)<br>
> + *<br>
> + */<br>
> +<br>
> +#include "nir.h"<br>
> +<br>
> +/*<br>
> + * Implements common subexpression elimination<br>
> + */<br>
<br>
</div></div>Stale comment.<br></blockquote><div><br></div><div>Fixed.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> +struct lower_phis_to_scalar_state {<br>
> + void *mem_ctx;<br>
> + void *dead_ctx;<br>
> +<br>
> + /* Hash table marking which phi nodes are scalarizable. The key is<br>
> + * pointers to phi instructions and the entry is either NULL for not<br>
> + * scalarizable or non-null for scalarizable.<br>
> + */<br>
> + struct hash_table *phi_table;<br>
> +};<br>
> +<br>
> +/* Determines if the given phi node should be lowered. The only phi nodes<br>
> + * we will scalarize at the moment are those where all of the sources are<br>
> + * scalarizable.<br>
> + */<br>
> +static bool<br>
> +should_lower_phi(nir_phi_instr *phi, struct lower_phis_to_scalar_state *state)<br>
> +{<br>
> + /* Already scalar */<br>
> + if (phi->dest.ssa.num_components == 1)<br>
> + return false;<br>
> +<br>
> + struct hash_entry *entry = _mesa_hash_table_search(state->phi_table, phi);<br>
> + if (entry)<br>
> + return entry->data != NULL;<br>
> +<br>
> + nir_foreach_phi_src(phi, src) {<br>
> + /* Don't know what to do with non-ssa sources */<br>
> + if (!src->src.is_ssa)<br>
> + return false;<br>
> + nir_instr *src_instr = src->src.ssa->parent_instr;<br>
> + switch (src_instr->type) {<br>
> + case nir_instr_type_alu: {<br>
> + nir_alu_instr *src_alu = nir_instr_as_alu(src_instr);<br>
> +<br>
> + /* ALU operations with output_size == 0 should be scalarized. We<br>
> + * will also see a bunch of vecN operations from scalarizing ALU<br>
> + * operations and, since they can easily be copy-propagated, they<br>
> + * are ok too.<br>
> + */<br>
> + return nir_op_infos[src_alu->op].output_size == 0 ||<br>
> + src_alu->op != nir_op_vec2 ||<br>
> + src_alu->op != nir_op_vec3 ||<br>
> + src_alu->op != nir_op_vec4;<br>
> + }<br>
> +<br>
> + case nir_instr_type_phi: {<br>
> + nir_phi_instr *src_phi = nir_instr_as_phi(src_instr);<br>
> +<br>
> + /* Insert an entry and mark it as scalarizable for now. That way<br>
> + * we don't recurse forever and a cycle in the depencence graph<br>
> + * won't automatically make us fail to scalarize.<br>
> + */<br>
> + entry = _mesa_hash_table_insert(state->phi_table, src_phi, (void *)1);<br>
<br>
</div></div>I expect "(void *)1" will give compiler warnings generally. "(void<br>
*)(uintptr_t)1" is the usual workaround.<br>
<br>
Useful comment, though!<br>
<span class=""><br>
> + bool scalarizable = should_lower_phi(src_phi, state);<br>
> + entry->data = (void *)scalarizable;<br>
<br>
</span>No need to cast to void *, since not C++.<br><div><div class="h5"></div></div></blockquote><div><br></div><div>I'm confused. You want more casting above but less here? Could you pleas be a little more specific.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
> +static bool<br>
> +lower_phis_to_scalar_block(nir_block *block, void *void_state)<br>
> +{<br>
> + struct lower_phis_to_scalar_state *state = void_state;<br>
> +<br>
> + /* Find the last phi node in the block */<br>
> + nir_phi_instr *last_phi = NULL;<br>
> + nir_foreach_instr(block, instr) {<br>
> + if (instr->type != nir_instr_type_phi)<br>
> + break;<br>
> +<br>
> + last_phi = nir_instr_as_phi(instr);<br>
> + }<br>
> +<br>
> + /* We have to handle the phi nodes in their own pass due to the way<br>
> + * we're modifying the linked list of instructions.<br>
> + */<br>
> + nir_foreach_instr_safe(block, instr) {<br>
> + if (instr->type != nir_instr_type_phi)<br>
> + break;<br>
> +<br>
> + nir_phi_instr *phi = nir_instr_as_phi(instr);<br>
> +<br>
> + if (!should_lower_phi(phi, state))<br>
> + continue;<br>
> +<br>
> + /* Create a vecN operation to combine the results. Most of these<br>
> + * will be redundant, but copy propagation should clean them up for<br>
> + * us. No need to add the complexity here.<br>
> + */<br>
> + nir_op vec_op;<br>
> + switch (phi->dest.ssa.num_components) {<br>
> + case 2: vec_op = nir_op_vec2; break;<br>
> + case 3: vec_op = nir_op_vec3; break;<br>
> + case 4: vec_op = nir_op_vec4; break;<br>
> + default: unreachable("Invalid number of components");<br>
> + }<br>
> +<br>
> + nir_alu_instr *vec = nir_alu_instr_create(state->mem_ctx, vec_op);<br>
> + vec->dest.dest.is_ssa = true;<br>
> + nir_ssa_def_init(&vec->instr, &vec->dest.dest.ssa,<br>
> + phi->dest.ssa.num_components, NULL);<br>
<br>
</div></div>nir_ssa_dest_init() instead? (throughout the file)<br></blockquote><div><br></div><div>Yeah. We didn't have that when I originally wrote this pass. Updated for the new stuff.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> + vec->dest.write_mask = (1 << phi->dest.ssa.num_components) - 1;<br>
> +<br>
> + for (unsigned i = 0; i < phi->dest.ssa.num_components; i++) {<br>
> + nir_phi_instr *new_phi = nir_phi_instr_create(state->mem_ctx);<br>
> + new_phi->dest.is_ssa = true;<br>
> + nir_ssa_def_init(&new_phi->instr, &new_phi->dest.ssa, 1, NULL);<br>
> +<br>
> + vec->src[i].src.is_ssa = true;<br>
> + vec->src[i].src.ssa = &new_phi->dest.ssa;<br>
> +<br>
> + nir_foreach_phi_src(phi, src) {<br>
> + /* We need to insert a mov to grab the i'th component of src */<br>
> + nir_alu_instr *mov = nir_alu_instr_create(state->mem_ctx,<br>
> + nir_op_imov);<br>
> + mov->dest.dest.is_ssa = true;<br>
> + nir_ssa_def_init(&mov->instr, &mov->dest.dest.ssa, 1, NULL);<br>
> + mov->dest.write_mask = 1;<br>
> + mov->src[0].src = nir_src_copy(src->src, state->mem_ctx);<br>
> + mov->src[0].swizzle[0] = i;<br>
> +<br>
> + /* Insert at the end of the predecessor but before the jump */<br>
> + nir_instr *pred_last_instr = nir_block_last_instr(src->pred);<br>
> + if (pred_last_instr && pred_last_instr->type == nir_instr_type_jump)<br>
> + nir_instr_insert_before(pred_last_instr, &mov->instr);<br>
> + else<br>
> + nir_instr_insert_after_block(src->pred, &mov->instr);<br>
<br>
</div></div>Couldn't you just insert your mov right after the def of the src? I'm<br>
confused.<span class=""><br></span></blockquote><div><br></div><div>Sure<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> +<br>
> + nir_phi_src *new_src = ralloc(state->mem_ctx, nir_phi_src);<br>
> + new_src->pred = src->pred;<br>
> + new_src->src.is_ssa = true;<br>
> + new_src->src.ssa = &mov->dest.dest.ssa;<br>
<br>
</span>new_src->src = nir_src_for_ssa(&mov->dest.dest.ssa);<br>
(and elsewhere in the file)<br></blockquote><div><br></div><div>Again, thanks for pointing that out. I've got that fixed now.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> +<br>
> + exec_list_push_tail(&new_phi->srcs, &new_src->node);<br>
> + }<br>
> +<br>
> + nir_instr_insert_before(&phi->instr, &new_phi->instr);<br>
> + }<br>
> +<br>
> + nir_instr_insert_after(&last_phi->instr, &vec->instr);<br>
> +<br>
> + nir_ssa_def_rewrite_uses(&phi->dest.ssa,<br>
> + nir_src_for_ssa(&vec->dest.dest.ssa),<br>
> + state->mem_ctx);<br>
> +<br>
> + ralloc_steal(state->dead_ctx, phi);<br>
> + nir_instr_remove(&phi->instr);<br>
> +<br>
> + /* We're using the safe iterator and inserting all the newly<br>
> + * scalarized phi nodes before their non-scalarized version so that's<br>
> + * ok. However, we are also inserting vec operations after all of<br>
> + * the last phi node so once we get here, we can't trust even the<br>
> + * safe iterator to stop properly. We have to break manually.<br>
> + */<br>
> + if (instr == &last_phi->instr)<br>
> + break;<br>
> + }<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> +static void<br>
> +lower_phis_to_scalar_impl(nir_function_impl *impl)<br>
> +{<br>
> + struct lower_phis_to_scalar_state state;<br>
> +<br>
> + state.mem_ctx = ralloc_parent(impl);<br>
> + state.dead_ctx = ralloc_context(NULL);<br>
> + state.phi_table = _mesa_hash_table_create(state.dead_ctx, _mesa_hash_pointer,<br>
> + _mesa_key_pointer_equal);<br>
> +<br>
> + nir_foreach_block(impl, lower_phis_to_scalar_block, &state);<br>
> +<br>
> + nir_metadata_preserve(impl, nir_metadata_block_index |<br>
> + nir_metadata_dominance);<br>
> +<br>
> + ralloc_free(state.dead_ctx);<br>
> +}<br>
<br>
</div></div>I just notied: When do I need to all nir_metadata_preserve()? I didn't<br>
in my pass.<br>
</blockquote></div><br></div></div>