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

[Connor Abbott]

On Fri, Sep 2, 2016 at 8:48 PM, Jason Ekstrand <jason at 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 <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
>
>