<div dir="auto"><div><div class="gmail_extra"><div class="gmail_quote" dir="auto">Looks functionally correct. I left a few simple comments.</div><div class="gmail_quote" dir="auto"> </div><div class="gmail_quote">On Jan 7, 2017 3:59 AM, "Timothy Arceri" <<a href="mailto:timothy.arceri@collabora.com">timothy.arceri@collabora.com</a>> wrote: <blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> This tries to move comparisons (a common source of boolean values) closer to their first use. For GPUs which use condition codes, this can eliminate a lot of temporary booleans and comparisons which reload the condition code register based on a boolean. V2: (Timothy Arceri) - fix move comparision for phis so we dont end up with: vec1 32 ssa_227 = phi block_34: ssa_1, block_38: ssa_240 vec1 32 ssa_235 = feq ssa_227, ssa_1 vec1 32 ssa_230 = phi block_34: ssa_221, block_38: ssa_235 - add nir_op_i2b/nir_op_f2b to the list of comparisons. Signed-off-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> Reviewed-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> [v1] --- src/compiler/Makefile.sources | 1 + src/compiler/nir/nir.h | 2 + src/compiler/nir/nir_opt_move_comparisons.c | 176 ++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+) create mode 100644 src/compiler/nir/nir_opt_move_comparisons.c diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources index 52f6e54..6da854e 100644 --- a/src/compiler/Makefile.sources +++ b/src/compiler/Makefile.sources @@ -245,6 +245,7 @@ NIR_FILES = \ nir/nir_opt_global_to_local.c \ nir/nir_opt_if.c \ nir/nir_opt_loop_unroll.c \ + nir/nir_opt_move_comparisons.c \ nir/nir_opt_peephole_select.c \ nir/nir_opt_remove_phis.c \ nir/nir_opt_trivial_continues.c \ diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h index d17924c..325d73b 100644 --- a/src/compiler/nir/nir.h +++ b/src/compiler/nir/nir.h @@ -2563,6 +2563,8 @@ bool nir_opt_if(nir_shader *shader); bool nir_opt_loop_unroll(nir_shader *shader, nir_variable_mode indirect_mask); +bool nir_opt_move_comparisons(nir_shader *shader); + bool nir_opt_peephole_select(nir_shader *shader, unsigned limit); bool nir_opt_remove_phis(nir_shader *shader); diff --git a/src/compiler/nir/nir_opt_move_comparisons.c b/src/compiler/nir/nir_opt_move_comparisons.c new file mode 100644 index 0000000..e6bee94 --- /dev/null +++ b/src/compiler/nir/nir_opt_move_comparisons.c @@ -0,0 +1,176 @@ +/* + * 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" + +/** + * \file nir_opt_move_comparisons.c + * + * This pass moves ALU comparison operations just before their first use. + * + * It only moves instructions within a single basic block; cross-block + * movement is left to global code motion. + * + * Many GPUs generate condition codes for comparisons, and use predication + * for conditional selects and control flow. In a sequence such as: + * + * vec1 32 ssa_1 = flt a b + * <some other operations> + * vec1 32 ssa_2 = bcsel ssa_1 c d + * + * the backend would likely do the comparison, producing condition codes, + * then save those to a boolean value. The intervening operations might + * trash the condition codes. Then, in order to do the bcsel, it would + * need to re-populate the condition code register based on the boolean. + * + * By moving the comparison just before the bcsel, the condition codes could + * be used directly. This eliminates the need to reload them from the boolean + * (generally eliminating an instruction). It may also eliminate the need to + * create a boolean value altogether (unless it's used elsewhere), which could + * lower register pressure. + */ + +static bool +is_comparison(nir_op op) +{ + switch (op) { + case nir_op_flt: + case nir_op_fge: + case nir_op_feq: + case nir_op_fne: + case nir_op_ilt: + case nir_op_ult: + case nir_op_ige: + case nir_op_uge: + case nir_op_ieq: + case nir_op_ine: + case nir_op_i2b: + case nir_op_f2b: + return true; + default: + return false; + } +} + +static bool +move_comparison_source(nir_src *src, nir_block *block, nir_instr *before) +{ + if (src->is_ssa && src->ssa->parent_instr->block == block && + src->ssa->parent_instr->type == nir_instr_type_alu && + is_comparison(nir_instr_as_alu(src->ssa->parent_instr)->op) && + (!before || before->type != nir_instr_type_phi)) { + + struct exec_node *src_node = &src->ssa->parent_instr->node;</blockquote></div></div></div><div dir="auto"> </div><div dir="auto">I think I would mildly prefer this to be "nir_instr *src_instr". It means a bit more typing but the thing you are really doing is adding/removing an instruction not a node.</div><div dir="auto"></div><div dir="auto"> </div><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + exec_node_remove(src_node); + + if (before) + exec_node_insert_node_before(&before->node, src_node); + else + exec_list_push_tail(&block->instr_list, src_node); + + return true; + } + + return false; +} + +/* nir_foreach_src callback boilerplate */ +struct nomc_tuple +{ + nir_instr *instr; + bool progress; +};</blockquote></div></div></div><div dir="auto"> </div><div dir="auto">nir_src has a parent_instr field so you can pull everything other then the progress boolean out of the SRC and you don't need this struct.</div><div dir="auto"> </div><div dir="auto"></div><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + +static bool +move_comparison_source_cb(nir_src *src, void *data) +{ + struct nomc_tuple *tuple = data; + + if (move_comparison_source(src, tuple->instr->block, tuple->instr)) + tuple->progress = true; + + return true; /* nir_foreach_src should keep going */ +} + +static bool +move_comparisons(nir_block *block) +{ + bool progress = false; + + /* We use a simple approach: walk instructions backwards. + * + * If the instruction's source is a comparison from the same block, + * simply move it here. This may break SSA if it's used earlier in + * the block as well. However, as we walk backwards, we'll find the + * earlier use and move it again, further up. It eventually ends up + * dominating all uses again, restoring SSA form. + * + * Before walking instructions, we consider the if-condition at the + * end of the block, if one exists. It's effectively a use at the + * bottom of the block. + */ + nir_if *iff = nir_block_get_following_if(block); + if (iff) { + progress |= move_comparison_source(&iff->condition, block, NULL); + } + + nir_foreach_instr_reverse(instr, block) { + if (instr->type == nir_instr_type_alu) { + /* Walk ALU instruction sources backwards so that bcsel's boolean + * condition is processed last. + */ + nir_alu_instr *alu = nir_instr_as_alu(instr); + for (int i = nir_op_infos[alu->op].num_inputs - 1; i >= 0; i--) { + progress |= move_comparison_source(&alu->src[i].src, + block, instr); + } + } else { + struct nomc_tuple tuple = { instr, false }; + nir_foreach_src(instr, move_comparison_source_cb, &tuple); + progress |= tuple.progress; + } + } + + return progress; +} + +bool +nir_opt_move_comparisons(nir_shader *shader) +{ + bool progress = false; + + nir_foreach_function(func, shader) { + if (func->impl) { </blockquote></div></div></div><div dir="auto"> </div><div dir="auto">I tend to prefer "if (!func->impl) continue;" myself but it doesn't really matter.</div><div dir="auto"> </div><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + nir_foreach_block(block, func->impl) { + if (move_comparisons(block)) { + nir_metadata_preserve(func->impl, nir_metadata_block_index | + nir_metadata_dominance | + nir_metadata_live_ssa_defs); + progress = true; + } + } + } + } + + return progress; +} -- 2.9.3 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div></div>