On 06/11/2018 08:23 AM, Rob Clark wrote: > Run this pass late (after opt loop) to move load_const instructions back > into the basic blocks which use the result, in cases where a load_const > is only consumed in a single block. > > This helps reduce register usage in cases where the backend driver > cannot lower the load_const to a uniform. > > v2: use nir_dominance_lca to figure out a better candidate block to > move the load_const to, when all it's uses are not in the same > block > > Signed-off-by: Rob Clark <[email protected]>
I tried enabling this, and the results were a bit different than last time. A bunch of shaders were hurt for spills and fills... I may end up tweaking this a bit before we enable it in i965. Reviewed-by: Ian Romanick <[email protected]> > --- > Ian, I played around with the following hack on top of this: > ----------- > diff --git a/src/compiler/nir/nir_move_load_const.c > b/src/compiler/nir/nir_move_load_const.c > index abc53fdce6a..7a84cce2da1 100644 > --- a/src/compiler/nir/nir_move_load_const.c > +++ b/src/compiler/nir/nir_move_load_const.c > @@ -54,6 +54,10 @@ get_preferred_block(nir_ssa_def *def) > nir_instr *instr = use->parent_instr; > nir_block *use_block = instr->block; > > + // XXX hack: > + if (use_block->imm_dom && exec_list_length(&use_block->instr_list) < > 3) > + use_block = use_block->imm_dom; > + > /* > * Kind of an ugly special-case, but phi instructions > * need to appear first in the block, so by definition > ----------- > > I just guessed that threshold at random. But it does seem to help > improve the "total cycles in shared programs".. although my shaderdb > numbers don't quite match yours (I guess I have an older copy of the > closed db?) > > However I guess it is probably best to leave it to the backend compiler > to move load_const to an earlier block when needed to hide latency. > > src/compiler/Makefile.sources | 1 + > src/compiler/nir/meson.build | 1 + > src/compiler/nir/nir.h | 1 + > src/compiler/nir/nir_move_load_const.c | 141 +++++++++++++++++++++++++ > 4 files changed, 144 insertions(+) > create mode 100644 src/compiler/nir/nir_move_load_const.c > > diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources > index d629c2b8ec8..252cbed0d26 100644 > --- a/src/compiler/Makefile.sources > +++ b/src/compiler/Makefile.sources > @@ -254,6 +254,7 @@ NIR_FILES = \ > nir/nir_lower_wpos_center.c \ > nir/nir_lower_wpos_ytransform.c \ > nir/nir_metadata.c \ > + nir/nir_move_load_const.c \ > nir/nir_move_vec_src_uses_to_dest.c \ > nir/nir_normalize_cubemap_coords.c \ > nir/nir_opt_conditional_discard.c \ > diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build > index 598c68aff9f..2b5da68c78b 100644 > --- a/src/compiler/nir/meson.build > +++ b/src/compiler/nir/meson.build > @@ -145,6 +145,7 @@ files_libnir = files( > 'nir_lower_wpos_ytransform.c', > 'nir_lower_bit_size.c', > 'nir_metadata.c', > + 'nir_move_load_const.c', > 'nir_move_vec_src_uses_to_dest.c', > 'nir_normalize_cubemap_coords.c', > 'nir_opt_conditional_discard.c', > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h > index bb477742dc6..2d620454796 100644 > --- a/src/compiler/nir/nir.h > +++ b/src/compiler/nir/nir.h > @@ -2630,6 +2630,7 @@ bool nir_remove_dead_variables(nir_shader *shader, > nir_variable_mode modes); > bool nir_lower_constant_initializers(nir_shader *shader, > nir_variable_mode modes); > > +bool nir_move_load_const(nir_shader *shader); > bool nir_move_vec_src_uses_to_dest(nir_shader *shader); > bool nir_lower_vec_to_movs(nir_shader *shader); > void nir_lower_alpha_test(nir_shader *shader, enum compare_func func, > diff --git a/src/compiler/nir/nir_move_load_const.c > b/src/compiler/nir/nir_move_load_const.c > new file mode 100644 > index 00000000000..abc53fdce6a > --- /dev/null > +++ b/src/compiler/nir/nir_move_load_const.c > @@ -0,0 +1,141 @@ > +/* > + * Copyright © 2018 Red Hat > + * > + * 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. > + * > + * Authors: > + * Rob Clark ([email protected]> > + * > + */ > + > +#include "nir.h" > + > + > +/* > + * A simple pass that moves load_const's into consuming block if > + * they are only consumed in a single block, to try to counter- > + * act nir's tendency to move all load_const to the top of the > + * first block. > + */ > + > +/* iterate a ssa def's use's and try to find a more optimal block to > + * move it to, using the dominance tree. In short, if all of the uses > + * are contained in a single block, the load will be moved there, > + * otherwise it will be move to the least common ancestor block of all > + * the uses > + */ > +static nir_block * > +get_preferred_block(nir_ssa_def *def) > +{ > + nir_block *lca = NULL; > + > + /* hmm, probably ignore if-uses: */ > + if (!list_empty(&def->if_uses)) > + return NULL; > + > + nir_foreach_use(use, def) { > + nir_instr *instr = use->parent_instr; > + nir_block *use_block = instr->block; > + > + /* > + * Kind of an ugly special-case, but phi instructions > + * need to appear first in the block, so by definition > + * we can't move a load_immed into a block where it is > + * consumed by a phi instruction. We could conceivably > + * move it into a dominator block. > + */ > + if (instr->type == nir_instr_type_phi) { > + nir_phi_instr *phi = nir_instr_as_phi(instr); > + nir_block *phi_lca = NULL; > + nir_foreach_phi_src(src, phi) > + phi_lca = nir_dominance_lca(phi_lca, src->pred); > + use_block = phi_lca; > + } > + > + lca = nir_dominance_lca(lca, use_block); > + } > + > + return lca; > +} > + > +/* insert before first non-phi instruction: */ > +static void > +insert_after_phi(nir_instr *instr, nir_block *block) > +{ > + nir_foreach_instr(instr2, block) { > + if (instr2->type == nir_instr_type_phi) > + continue; > + > + exec_node_insert_node_before(&instr2->node, > + &instr->node); > + > + return; > + } > + > + /* if haven't inserted it, push to tail (ie. empty block or possibly > + * a block only containing phi's?) > + */ > + exec_list_push_tail(&block->instr_list, &instr->node); > +} > + > +bool > +nir_move_load_const(nir_shader *shader) > +{ > + bool progress = false; > + > + nir_foreach_function(function, shader) { > + if (!function->impl) > + continue; > + > + nir_foreach_block(block, function->impl) { > + nir_metadata_require(function->impl, > + nir_metadata_block_index | > nir_metadata_dominance); > + > + nir_foreach_instr_safe(instr, block) { > + if (instr->type != nir_instr_type_load_const) > + continue; > + > + nir_load_const_instr *load = > + nir_instr_as_load_const(instr); > + nir_block *use_block = > + get_preferred_block(&load->def); > + > + if (!use_block) > + continue; > + > + if (use_block == load->instr.block) > + continue; > + > + exec_node_remove(&load->instr.node); > + > + insert_after_phi(&load->instr, use_block); > + > + load->instr.block = use_block; > + > + progress = true; > + } > + } > + > + nir_metadata_preserve(function->impl, > + nir_metadata_block_index | > nir_metadata_dominance); > + } > + > + return progress; > +} > _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
