On Fri, Sep 2, 2016 at 8:48 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> 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.
Yeah, if we implement that then we can just get rid of the whole thing.
>
> 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
