[Mesa-dev] [PATCH 2/4] nvc0/ir: don't dual issue instructions which depend on each other

[karol herbst]

2016-08-13 18:17 GMT+02:00 Ilia Mirkin <imirkin at alum.mit.edu>:
> On Sat, Aug 13, 2016 at 6:02 AM, Karol Herbst <karolherbst at gmail.com> wrote:
>> no changes without a dual_issue pass
>>
>> changes with for ./GpuTest /test=pixmark_piano /benchmark /no_scorebox /msaa=0
>> /benchmark_duration_ms=60000 /width=1024 /height=640:
>>
>> inst_executed: 1.03G
>> inst_issued1: 538M -> 535M
>> inst_issued2: 251M -> 254M
>>
>> score: 1038 -> 1052
>>
>> Signed-off-by: Karol Herbst <karolherbst at gmail.com>
>> ---
>>  src/gallium/drivers/nouveau/codegen/nv50_ir.cpp             | 11 +++++++++--
>>  src/gallium/drivers/nouveau/codegen/nv50_ir.h               |  1 +
>>  src/gallium/drivers/nouveau/codegen/nv50_ir_target_nvc0.cpp |  4 ++++
>>  3 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
>> index 179ad0b..7a90cb7 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir.cpp
>> @@ -893,12 +893,19 @@ insnCheckCommutationDefDef(const Instruction *a, const Instruction *b)
>>  bool
>>  Instruction::isCommutationLegal(const Instruction *i) const
>>  {
>> -   bool ret = insnCheckCommutationDefDef(this, i);
>> -   ret = ret && insnCheckCommutationDefSrc(this, i);
>> +   bool ret = !i->dependsOn(this);
>>     ret = ret && insnCheckCommutationDefSrc(i, this);
>
> This condition is covered by your new dependsOn() call. Did you mean
> dependsOn to have something different? I haven't really thought
> carefully about this yet.

dependsOn should just check whether the second instruction depend on
the first one, where isCommutationLegal also checks if you could swap
both.

mad $r1 $r2 $r3 $r4
add $r4 $r2 $r2

=>

add.isCommutationLegal(mad) : false
add.dependsOn(mad) : false

those instructions can be dual issued, but the isCommutationLegal
method was too strict about the check.

I hope this is enough to clear it up.

>
>>     return ret;
>>  }
>>
>> +bool
>> +Instruction::dependsOn(const Instruction *i) const
>> +{
>> +   bool ret = insnCheckCommutationDefDef(this, i);
>> +   ret = ret && insnCheckCommutationDefSrc(i, this);
>> +   return !ret;
>> +}
>> +
>>  TexInstruction::TexInstruction(Function *fn, operation op)
>>     : Instruction(fn, op, TYPE_F32)
>>  {
>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir.h b/src/gallium/drivers/nouveau/codegen/nv50_ir.h
>> index 6d2ee8b..d81fca9 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir.h
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir.h
>> @@ -831,6 +831,7 @@ public:
>>     bool isDead() const;
>>     bool isNop() const;
>>     bool isCommutationLegal(const Instruction *) const; // must be adjacent !
>> +   bool dependsOn(const Instruction *) const; // weaker form of isCommutationLegal
>>     bool isActionEqual(const Instruction *) const;
>>     bool isResultEqual(const Instruction *) const;
>>
>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nvc0.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nvc0.cpp
>> index faf2121..8ce8c19 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nvc0.cpp
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_target_nvc0.cpp
>> @@ -620,6 +620,10 @@ bool TargetNVC0::canDualIssue(const Instruction *a, const Instruction *b) const
>>        // not if the 2nd instruction isn't necessarily executed
>>        if (clA == OPCLASS_TEXTURE || clA == OPCLASS_FLOW)
>>           return false;
>> +
>> +      if (b->dependsOn(a))
>> +         return false;
>> +
>>        // anything with MOV
>>        if (a->op == OP_MOV || b->op == OP_MOV)
>>           return true;
>> --
>> 2.9.2
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev