[Mesa-dev] [PATCH] nv50/ir: replace the right def in global cse

[Connor Abbott]

On Mon, Jan 11, 2016 at 11:44 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> Actually this is still a little bogus... We'd have to ensure that
> *all* of the defs are used in this bb or later. I'll try to come up
> with something that still maintains some of the potential benefit but
> is correct.

I don't know about the details of nv50, but I can say that we do a
pretty similar thing in NIR (removing phis whose sources are all the
same), and I don't see why it would be wrong. The SSA value must
dominate the phi node since it dominates all of its predecessors, so
it must dominate all uses of the phi, which means it should be ok to
replace uses of the phi with uses of the value. Of course, if the
source of the phi isn't an SSA value, then it won't work, but I
presume you don't have to handle that case here.

>
> On Mon, Jan 11, 2016 at 10:06 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> GlobalCSE looks for phi nodes with identically-defined sources, and
>> replaces them with one of the (identical) instructions.
>>
>> However the defining instruction might have multiple defs, so we have to
>> replace the one that the original phi node was pointing at.
>>
>> Observed when playing around with st_glsl_to_tgsi optimization settings,
>> this produced a shader where a texture op was being propagated. We would
>> always end up setting its first def instead of the one the phi node was
>> pointing at.
>>
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> ---
>>  .../drivers/nouveau/codegen/nv50_ir_peephole.cpp    | 21 +++++++++++++--------
>>  1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>> index 24caf18..aa56078 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp
>> @@ -3028,7 +3028,7 @@ bool
>>  GlobalCSE::visit(BasicBlock *bb)
>>  {
>>     Instruction *phi, *next, *ik;
>> -   int s;
>> +   int s, d;
>>
>>     // TODO: maybe do this with OP_UNION, too
>>
>> @@ -3048,13 +3048,18 @@ GlobalCSE::visit(BasicBlock *bb)
>>        }
>>        if (!phi->srcExists(s)) {
>>           Instruction *entry = bb->getEntry();
>> -         ik->bb->remove(ik);
>> -         if (!entry || entry->op != OP_JOIN)
>> -            bb->insertHead(ik);
>> -         else
>> -            bb->insertAfter(entry, ik);
>> -         ik->setDef(0, phi->getDef(0));
>> -         delete_Instruction(prog, phi);
>> +         for (d = 0; ik->defExists(d); ++d)
>> +            if (ik->getDef(d) == phi->getSrc(0))
>> +               break;
>> +         if (ik->defExists(d)) {
>> +            ik->bb->remove(ik);
>> +            if (!entry || entry->op != OP_JOIN)
>> +               bb->insertHead(ik);
>> +            else
>> +               bb->insertAfter(entry, ik);
>> +            ik->setDef(d, phi->getDef(0));
>> +            delete_Instruction(prog, phi);
>> +         }
>>        }
>>     }
>>
>> --
>> 2.4.10
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev