2016-08-13 19:27 GMT+02:00 Ilia Mirkin <imir...@alum.mit.edu>:
> On Sat, Aug 13, 2016 at 1:24 PM, karol herbst <karolher...@gmail.com> wrote:
>> 2016-08-13 18:17 GMT+02:00 Ilia Mirkin <imir...@alum.mit.edu>:
>>> On Sat, Aug 13, 2016 at 6:02 AM, Karol Herbst <karolher...@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 <karolher...@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@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
