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

[Ilia Mirkin]

On Sat, Aug 13, 2016 at 1:24 PM, karol herbst <karolherbst at gmail.com> wrote:
> 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.

I think you missed my point.

dependsOn == !(DefDef(this, i) && DefSrc(i, this))

But then you check DefSrc(i, this) again in isCommutationLegal, so it becomes

!dependsOn && DefSrc(i, this), i.e.

DefDef(this, i) && DefSrc(i, this) && DefSrc(i, this)

Either you meant a different condition, or there's redundant code.

>
>>
>>>     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