[Mesa-dev] [PATCH] nir: add pass to move load_const
Rob Clark
robdclark at gmail.com
Tue Jun 19 00:03:01 UTC 2018
On Mon, Jun 18, 2018 at 6:36 PM, Ian Romanick <idr at freedesktop.org> wrote:
> 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 <robdclark at gmail.com>
>
> 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.
>
just curious, but is "fills" the inverse of "spills" (ie. restoring
spilled registers)?
It is a bit strange that the newer version effected spills, since the
main diff is it got a bit better at moving some load_const that it
couldn't before (so I'd expect more a cycles difference.. and that is
probably, like you mentioned earlier, something to deal with in
backend scheduler)..
anyways, tweak it more in future to make more useful to more drivers
seems like a legit approach.. I think if nothing else in the current
form it should help driver with less clever backend compilers.
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
thanks
BR,
-R
>> ---
>> 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 (robdclark at gmail.com>
>> + *
>> + */
>> +
>> +#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;
>> +}
>>
>
More information about the mesa-dev
mailing list