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

[karol herbst]

2016-08-13 19:27 GMT+02:00 Ilia Mirkin <imirkin at alum.mit.edu>:
> 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.
>

I know what you mean, I mad the same mistake, too, but this way it is right:

instruction a,b;

a->isCommutationLegal(b)

evaluates to

bool ret = !b->dependsOn(a);
ret = ret && insnCheckCommutationDefSrc(b, a);

where

b->dependsOn(a);

evaluates to

bool ret = insnCheckCommutationDefDef(b, a);
ret = ret && insnCheckCommutationDefSrc(a, b);

merged together:

bool ret = !(
   bool ret = insnCheckCommutationDefDef(b, a);
   ret = ret && insnCheckCommutationDefSrc(a, b);
   return !ret;
)
ret = ret && insnCheckCommutationDefSrc(b, a);

which is

bool ret = insnCheckCommutationDefDef(b, a);
ret = ret && insnCheckCommutationDefSrc(a, b);
ret = ret && insnCheckCommutationDefSrc(b, a);

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