[Mesa-dev] [PATCH 2/2] nir: fix bug with moves in nir_opt_remove_phis()

[Jason Ekstrand]

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 <cwabbott0 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160902/3562f7ae/attachment.html>