[Mesa-dev] [PATCH 2/6] nv50/ir: add LIMM form of mad to gm107

[Karol Herbst]

2016-10-08 18:12 GMT+02:00 Samuel Pitoiset <samuel.pitoiset at gmail.com>:
> Usually we prefix with gm107/ir, gk110/ir, etc...
>
> More comments below.
>
> On 10/08/2016 05:43 PM, Karol Herbst wrote:
>>
>> Signed-off-by: Karol Herbst <karolherbst at gmail.com>
>> ---
>>  .../drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp | 32
>> ++++++++++++++++------
>>  1 file changed, 23 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
>> b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
>> index 3fedafd..d084e07 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_emit_gm107.cpp
>> @@ -1306,7 +1306,7 @@ CodeEmitterGM107::emitFMUL()
>>  void
>>  CodeEmitterGM107::emitFFMA()
>>  {
>> -   /*XXX: ffma32i exists, but not using it as third src overlaps dst */
>> +   bool isLongIMMD = false;
>>     switch(insn->src(2).getFile()) {
>>     case FILE_GPR:
>>        switch (insn->src(1).getFile()) {
>> @@ -1319,14 +1319,21 @@ CodeEmitterGM107::emitFFMA()
>>           emitCBUF(0x22, -1, 0x14, 16, 2, insn->src(1));
>>           break;
>>        case FILE_IMMEDIATE:
>> -         emitInsn(0x32800000);
>> -         emitIMMD(0x14, 19, insn->src(1));
>> +         if (longIMMD(insn->getSrc(1))) {
>> +            isLongIMMD = true;
>> +            emitInsn(0x0c000000);
>> +            emitIMMD(0x14, 32, insn->src(1));
>> +         } else {
>> +            emitInsn(0x32800000);
>> +            emitIMMD(0x14, 19, insn->src(1));
>> +         }
>>           break;
>>        default:
>>           assert(!"bad src1 file");
>>           break;
>>        }
>> -      emitGPR (0x27, insn->src(2));
>> +      if (!isLongIMMD)
>> +         emitGPR (0x27, insn->src(2));
>>        break;
>>     case FILE_MEMORY_CONST:
>>        emitInsn(0x51800000);
>> @@ -1337,12 +1344,19 @@ CodeEmitterGM107::emitFFMA()
>>        assert(!"bad src2 file");
>>        break;
>>     }
>> -   emitRND (0x33);
>> -   emitSAT (0x32);
>> -   emitNEG (0x31, insn->src(2));
>> -   emitNEG2(0x30, insn->src(0), insn->src(1));
>> -   emitCC  (0x2f);
>> +   if (isLongIMMD) {
>> +      emitCC  (0x34);
>> +      emitSAT (0x37);
>> +      emitNEG2(0x38, insn->src(0), insn->src(1));
>> +      emitNEG (0x39, insn->src(2));
>> +   } else {
>> +      emitCC  (0x2f);
>> +      emitNEG2(0x30, insn->src(0), insn->src(1));
>> +      emitNEG (0x31, insn->src(2));
>> +      emitSAT (0x32);
>> +   }
>
>
> Please re-order.
>
> Also, because FFMA32I modifiers are really different than FFMA, it would
> make more sense to add emitFFMA32I() and select the good one regarding the
> LIMM form or not.
>

currently other emit methods do the same and this would also move more
logic inside the big switch. I think a better solution would be to
split inside emitFFMA and call emitFFMA32I() or emitFFMASomething()
from there. But I would leave it as is, just because it is the same
everywhere else.

> Otherwise, I don't see any restrictions which make sure that def == src2 for
> FFMA32I...
>
> If you check that later in the series, this patch has to be *after*.
>
>>
>> +   emitRND(0x33);
>>     emitFMZ(0x35, 2);
>>     emitGPR(0x08, insn->src(0));
>>     emitGPR(0x00, insn->def(0));
>>
>