[Mesa-dev] [PATCH v3 3/4] nir/phi-builder: Set the value in the block when creating a phi

[Connor Abbott]

On Fri, Dec 16, 2016 at 3:00 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> Ken, Eric,
>
> Any thoughts on whether or not this should go to stable?  It does fix a real
> bug but the result of the bug is that we end up generating a bunch of
> duplicate phi nodes instead of just one for the block.  In the end it
> doesn't matter since CSE cleans them up.

Not sure we should really bother with sending this to stable, since as
you said it doesn't result in any actual bug -- just more time spent
generating and cleaning up redundant phi nodes.

>
> --Jason
>
> On Fri, Dec 16, 2016 at 11:05 AM, Jason Ekstrand <jason at jlekstrand.net>
> wrote:
>>
>> After we figure out the value that we are going to return, we have a
>> loop that walks up the dominance tree and sets the value in each of the
>> blocks that doesn't have one yet.  In the case of the phi, the def is
>> set to NEEDS_PHI not NULL, so the last one where the phi node actually
>> goes never gets filled out.  This can lead to duplicating the phi node
>> unnecessarily.
>> ---
>>  src/compiler/nir/nir_phi_builder.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/compiler/nir/nir_phi_builder.c
>> b/src/compiler/nir/nir_phi_builder.c
>> index 6b4b693..acfc771 100644
>> --- a/src/compiler/nir/nir_phi_builder.c
>> +++ b/src/compiler/nir/nir_phi_builder.c
>> @@ -216,7 +216,7 @@ nir_phi_builder_value_get_block_def(struct
>> nir_phi_builder_value *val,
>>                          val->bit_size, NULL);
>>        phi->instr.block = dom;
>>        exec_list_push_tail(&val->phis, &phi->instr.node);
>> -      def = &phi->dest.ssa;
>> +      def = val->defs[dom->index] = &phi->dest.ssa;
>>     } else {
>>        /* In this case, we have an actual SSA def.  It's either the result
>> of a
>>         * phi node created by the case above or one passed to us through
>> --
>> 2.5.0.400.gff86faf
>>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>