On Tue, 2016-08-30 at 14:06 +1000, Timothy Arceri wrote:
> On Mon, 2016-08-29 at 20:34 -0400, Connor Abbott wrote:
> >
> > On Mon, Aug 29, 2016 at 12:59 AM, Timothy Arceri
> > <timothy.arc...@collabora.com> wrote:
> > >
> > >
> > > ---
> > > src/compiler/Makefile.sources | 1 +
> > > src/compiler/nir/nir.h | 2 +
> > > src/compiler/nir/nir_opt_loop_unroll.c | 394
> > > +++++++++++++++++++++++++++++++++
> > > 3 files changed, 397 insertions(+)
> > > create mode 100644 src/compiler/nir/nir_opt_loop_unroll.c
> > >
> > > diff --git a/src/compiler/Makefile.sources
> > > b/src/compiler/Makefile.sources
> > > index 79de484..a9f104d 100644
> > > --- a/src/compiler/Makefile.sources
> > > +++ b/src/compiler/Makefile.sources
> > > @@ -231,6 +231,7 @@ NIR_FILES = \
> > > nir/nir_opt_dead_cf.c \
> > > nir/nir_opt_gcm.c \
> > > nir/nir_opt_global_to_local.c \
> > > + nir/nir_opt_loop_unroll.c \
> > > nir/nir_opt_peephole_select.c \
> > > nir/nir_opt_remove_phis.c \
> > > nir/nir_opt_undef.c \
> > > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> > > index 9083bd0..81d9dfc 100644
> > > --- a/src/compiler/nir/nir.h
> > > +++ b/src/compiler/nir/nir.h
> > > @@ -2676,6 +2676,8 @@ bool nir_opt_dead_cf(nir_shader *shader);
> > >
> > > void nir_opt_gcm(nir_shader *shader);
> > >
> > > +bool nir_opt_loop_unroll(nir_shader *shader);
> > > +
> > > bool nir_opt_peephole_select(nir_shader *shader);
> > >
> > > bool nir_opt_remove_phis(nir_shader *shader);
> > > diff --git a/src/compiler/nir/nir_opt_loop_unroll.c
> > > b/src/compiler/nir/nir_opt_loop_unroll.c
> > > new file mode 100644
> > > index 0000000..22530c9
> > > --- /dev/null
> > > +++ b/src/compiler/nir/nir_opt_loop_unroll.c
> > > @@ -0,0 +1,394 @@
> > > +/*
> > > + * 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"
> > > +#include "nir_builder.h"
> > > +#include "nir_control_flow.h"
> > > +
> > > +typedef struct {
> > > + /* Array of loops */
> > > + nir_loop *loops;
> > > +
> > > + /* Array of unroll factors */
> > > + int *factors;
> > > +} unroll_vector;
> > > +
> > > +typedef struct {
> > > + /* Array of loop infos for the loop nest */
> > > + nir_loop_info *li;
> > > +
> > > + /* List of unroll vectors */
> > > + struct list_head unroll_vectors;
> > > +
> > > + nir_shader_compiler_options *options;
> > > +} loop_unroll_state;
> > > +
> > > +static void
> > > +extract_loop_body(nir_cf_list *extracted, nir_cf_node *node,
> > > nir_shader *ns)
> >
> > The shader argument is unused, you can delete it.
> >
> > >
> > >
> > > +{
> > > + nir_cf_node *end = node;
> > > + while (!nir_cf_node_is_last(end))
> > > + end = nir_cf_node_next(end);
> > > +
> > > + nir_cf_loop_list_extract(extracted, node, end);
> > > +}
> > > +
> > > +static void
> > > +clone_list(nir_shader *ns, nir_loop *loop, nir_cf_list
> > > *src_cf_list,
> > > + nir_cf_list *cloned_cf_list, struct hash_table
> > > *remap_table,
> > > + struct hash_table *phi_remap)
> > > +{
> > > + /* Dest list needs to at least have one block */
> > > + nir_block *nblk = nir_block_create(ns);
> > > + nblk->cf_node.parent = loop->cf_node.parent;
> > > + exec_list_push_tail(&cloned_cf_list->list, &nblk-
> > > >
> > > > cf_node.node);
> > > +
> > > + nir_clone_loop_list(&cloned_cf_list->list, &src_cf_list-
> > > >list,
> > > + remap_table, phi_remap, ns);
> > > +}
> > > +
> > > +static void
> > > +remove_unrolled_loop(nir_cf_node *loop, nir_block
> > > *block_before_loop,
> > > + struct hash_table *remap_table,
> > > + struct hash_table *phi_remap,
> > > + nir_function_impl *impl)
> > > +{
> > > + /* Fixup LCSSA-phi srcs */
> > > + nir_block *prev_block =
> > > nir_cf_node_as_block(nir_cf_node_prev(loop));
> > > + nir_cf_node *cf_node = nir_cf_node_next(loop);
> > > + assert(cf_node->type == nir_cf_node_block);
> > > +
> > > + nir_block *block = nir_cf_node_as_block(cf_node);
> > > + nir_foreach_instr_safe(instr, block) {
> > > + if (instr->type == nir_instr_type_phi) {
> > > + nir_phi_instr *phi = nir_instr_as_phi(instr);
> > > + assert(phi->is_lcssa_phi);
> > > +
> > > + if (nir_cf_node_as_loop(loop)->info->trip_count != 0) {
> > > + nir_foreach_phi_src_safe(src, phi) {
> > > + /* Update predecessor */
> > > + src->pred = prev_block;
> > > +
> > > + /* Update src */
> > > + struct hash_entry *hte =
> > > + _mesa_hash_table_search(phi_remap, src-
> > > >
> > > > src.ssa);
> > > + if (hte) {
> > > + nir_src new_src = nir_src_for_ssa((nir_ssa_def
> > > *) hte->data);
> > > + nir_instr_rewrite_src(instr, &src->src,
> > > new_src);
> > > + } else {
> > > + /* If the LCSSA src was not a phi fallback to
> > > following the
> > > + * the remappings in the remap table.
> > > + */
> > > + nir_ssa_def *ssa_def = src->src.ssa;
> > > + while ((hte =
> > > _mesa_hash_table_search(remap_table,
> > > + ssa_def)
> > > ))
> > > {
> > > + ssa_def = (nir_ssa_def *) hte->data;
> > > + }
> > > +
> > > + if (ssa_def != src->src.ssa){
> > > + nir_src new_src = nir_src_for_ssa(ssa_def);
> > > + nir_instr_rewrite_src(instr, &src->src,
> > > new_src);
> > > + }
> > > + }
> > > + }
> > > + }
> > > + nir_opt_remove_phi(phi);
> > > + } else {
> > > + /* There should be no more LCSSA-phis */
> > > + break;
> > > + }
> > > + }
> > > +
> > > + /* After cloning the loop and fixing up the LCSSA-phis we
> > > need
> > > to remove
> > > + * any obsolete phis at the beginning of the new unrolled
> > > block. To avoid
> > > + * failing validation.
> > > + */
> > > + nir_foreach_instr_safe(instr, block_before_loop) {
> > > + if (instr->type != nir_instr_type_phi)
> > > + continue;
> > > +
> > > + /* Replace phi */
> > > + nir_phi_instr *phi = nir_instr_as_phi(instr);
> > > + nir_opt_remove_phi(phi);
> > > + }
> > > +
> > > + /* Remove the loop */
> > > + nir_loop *nir_loop = nir_cf_node_as_loop(loop);
> > > + foreach_list_typed(nir_cf_node, child, node, &nir_loop->body)
> > > + nir_cleanup_cf_node(child, impl, false);
> > > +
> > > + exec_node_remove(&loop->node);
> > > +
> > > + stitch_blocks(prev_block, block);
> >
> > oof... I think you can just use nir_cf_node_delete() here. No need
> > to
> > use private CF functions.
>
>
> Again the cf helper functions try to be to smart.
> nir_cf_node_delete()
> ends up trying to unlink the jump and falls over.
>
>
> >
> >
> > >
> > >
> > > +}
> > > +
> > > +/**
> > > + * Unroll a loop which does not contain any jumps. For example,
> > > if the input
> > > + * is:
> > > + *
> > > + * (loop (...) ...instrs...)
> > > + *
> > > + * And the iteration count is 3, the output will be:
> > > + *
> > > + * ...instrs... ...instrs... ...instrs...
> > > + */
> > > +static void
> > > +simple_unroll(nir_function *fn, nir_loop *loop, nir_builder *b)
> > > +{
> > > + nir_shader *ns = fn->shader;
> > > + nir_block *prev_block =
> > > + nir_cf_node_as_block(nir_cf_node_prev(&loop->cf_node));
> > > +
> > > + struct exec_node *loop_node = exec_list_get_head(&loop-
> > > >body);
> > > + nir_cf_node *lp_header_cf_node = exec_node_data(nir_cf_node,
> > > loop_node,
> > > + node);
> >
> > There's a helper for this, nir_loop_first_cf_node().
>
> Thanks will use.
>
> >
> >
> > >
> > >
> > > +
> > > + struct hash_table *remap_table =
> > > + _mesa_hash_table_create(NULL, _mesa_hash_pointer,
> > > + _mesa_key_pointer_equal);
> > > +
> > > + struct hash_table *phi_remap =
> > > + _mesa_hash_table_create(NULL, _mesa_hash_pointer,
> > > + _mesa_key_pointer_equal);
> > > +
> > > + /* Get the loop header this contains a bunch of phis and the
> > > loops
> > > + * conditional.
> > > + */
> > > + assert(lp_header_cf_node->type == nir_cf_node_block);
> > > + nir_block *loop_header_blk =
> > > nir_cf_node_as_block(lp_header_cf_node);
> > > +
> > > + /* Remove any phi preds that are from the loop itself
> > > nir_opt_remove_phi()
> > > + * will clean this up further for us.
> > > + */
> > > + nir_foreach_instr_safe(instr, loop_header_blk) {
> > > + if (instr->type != nir_instr_type_phi)
> > > + break;
> > > +
> > > + nir_phi_instr *phi = nir_instr_as_phi(instr);
> > > + nir_foreach_phi_src_safe(src, phi) {
> > > + /* Is the pred from the block itself? */
> > > + if (src->pred->index > phi->instr.block->index &&
> > > + src->pred->cf_node.parent == &loop->cf_node) {
> > > +
> > > + /* Store the phi dest and src to be used for
> > > remaping
> > > */
> > > + _mesa_hash_table_insert(phi_remap, &phi->dest.ssa,
> > > src->src.ssa);
> > > +
> > > + list_del(&src->src.use_link);
> > > + exec_node_remove(&src->node);
> > > + }
> > > + }
> > > + }
> > > +
> > > + /* Skip over if node (loop condition) and get the real loop
> > > body we need
> > > + * to do this now because we are about to start messing with
> > > the control
> > > + * flow.
> > > + */
> > > + nir_cf_node *if_node = nir_cf_node_next(lp_header_cf_node);
> > > + nir_cf_node *cf_node = nir_cf_node_next(if_node);
> > > + assert(cf_node->type == nir_cf_node_block);
> > > +
> > > + /* Pluck out the loop header */
> > > + nir_cf_list lp_header;
> > > + nir_cf_extract(&lp_header,
> > > nir_before_cf_node(lp_header_cf_node),
> > > + nir_after_cf_node(lp_header_cf_node));
> > > +
> > > + loop_node = exec_list_get_head(&loop->body);
> > > + lp_header_cf_node = exec_node_data(nir_cf_node, loop_node,
> > > node);
> >
> > again, nir_loop_first_cf_node()
> >
> > >
> > >
> > > +
> > > + /* Extract phis from loop header and place before the loop */
> > > + nir_cf_list lp_header_phis;
> > > + nir_cf_loop_list_extract(&lp_header_phis, lp_header_cf_node,
> > > + lp_header_cf_node);
> > > + b->cursor = nir_before_cf_node(&loop->cf_node);
> > > + nir_cf_reinsert(&lp_header_phis, b->cursor);
> >
> > Why do we need to do this? I thought we didn't need to touch the
> > phi's
> > anymore and we were just going to delete them.
>
> Can't the phi have more than one source from before the loop? e.g
>
> int i = 0;
> if (somthing)
> i = 1;
> else
> i = 2;
>
> for ( ; i < 5; i++)
> do_stuff(i);
Although I guess we probably wouldn't have a trip count for something
like that, so I guess maybe not.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
