<div dir="ltr"><div>I think this is conceptually correct but I haven't reviewed it for complete correctness yet or tested it. </div><div>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. </div></div><div class="gmail_extra"> <div class="gmail_quote">On Fri, Sep 2, 2016 at 4:28 PM, Connor Abbott <<a href="mailto:cwabbott0@gmail.com" target="_blank">cwabbott0@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div>