I think this is conceptually correct but I haven't reviewed it for complete
correctness yet or tested it.
That said, I think this is just a special case of the phi distribution pass
that we talked about on IRC a bit today. Maybe we should just go ahead and
implement that? For unary ALU operations, phi distribution is a clear win
and mov is definitely unary.
On Fri, Sep 2, 2016 at 4:28 PM, Connor Abbott <cwabbo...@gmail.com> wrote:
> In 144cbf8 ("nir: Make nir_opt_remove_phis see through moves."), Ken
> made nir_opt_remove_phis able to coalesce phi nodes whose sources are
> all moves with the same swizzle. However, he didn't add the logic
> necessary for handling the fact that the phi may now have multiple
> different sources, even though the sources point to the same thing. For
> example, if we had something like:
>
> if (...)
> a1 = b.yx;
> else
> a2 = b.yx;
> a = phi(a1, a2)
> ... = a
>
> then we would rewrite it to
>
> if (...)
> a1 = b.yx;
> else
> a2 = b.yx;
> ... = a1
>
> by picking a random phi source, which in this case is invalid because
> the source doesn't dominate the phi. Instead, we need to change it to:
>
> if (...)
> a1 = b.yx;
> else
> a2 = b.yx;
> a3 = b.yx;
> ... = a3;
> ---
>
> This is an alternative to Ken's patch to revert it, although it's
> totally untested (just compile tested).
>
> src/compiler/nir/nir_opt_remove_phis.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/src/compiler/nir/nir_opt_remove_phis.c
> b/src/compiler/nir/nir_opt_remove_phis.c
> index ee92fbe..acaa6e1 100644
> --- a/src/compiler/nir/nir_opt_remove_phis.c
> +++ b/src/compiler/nir/nir_opt_remove_phis.c
> @@ -26,6 +26,7 @@
> */
>
> #include "nir.h"
> +#include "nir_builder.h"
>
> static nir_alu_instr *
> get_parent_mov(nir_ssa_def *ssa)
> @@ -63,7 +64,7 @@ matching_mov(nir_alu_instr *mov1, nir_ssa_def *ssa)
> */
>
> static bool
> -remove_phis_block(nir_block *block)
> +remove_phis_block(nir_block *block, nir_builder *b)
> {
> bool progress = false;
>
> @@ -113,6 +114,21 @@ remove_phis_block(nir_block *block)
> */
> assert(def != NULL);
>
> + if (mov) {
> + /* If the sources were all mov's from the same source with the
> same
> + * swizzle, then we can't just pick a random move because it may
> not
> + * dominate the phi node. Instead, we need to emit our own move
> after
> + * the phi which uses the shared source, and rewrite uses of the
> phi
> + * to use the move instead. This is ok, because while the mov's
> may
> + * not all dominate the phi node, their shared source does.
> + */
> +
> + b->cursor = nir_after_phis(block);
> + def = mov->op == nir_op_imov ?
> + nir_imov_alu(b, mov->src[0], def->num_components) :
> + nir_fmov_alu(b, mov->src[0], def->num_components);
> + }
> +
> assert(phi->dest.is_ssa);
> nir_ssa_def_rewrite_uses(&phi->dest.ssa, nir_src_for_ssa(def));
> nir_instr_remove(instr);
> @@ -127,9 +143,11 @@ static bool
> remove_phis_impl(nir_function_impl *impl)
> {
> bool progress = false;
> + nir_builder bld;
> + nir_builder_init(&bld, impl);
>
> nir_foreach_block(block, impl) {
> - progress |= remove_phis_block(block);
> + progress |= remove_phis_block(block, &bld);
> }
>
> if (progress) {
> --
> 2.5.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
