You can specify extra instruction flags using the ISA parser without
having to have a specialized constructor. It's an input to the
InstObjParams object.
Gabe
On 03/08/12 14:52, Ali Saidi wrote:
>
>
> Hi Brian,
>
> First, thanks for noticing the bug and letting us know,
> and providing a fix. It's a big help. Looking at the code it appears as
> though BranchImmReg is only used by CBZ, CBNZ, so my first inclination
> is to just replace it with something like the following:
>
> diff -r
> 71fb98a8f332 src/arch/arm/isa/templates/branch.isa
> ---
> a/src/arch/arm/isa/templates/branch.isa Thu Mar 08 10:25:20 2012
> -0600
> +++ b/src/arch/arm/isa/templates/branch.isa Thu Mar 08 16:50:17
> 2012 -0600
> @@ -212,6 +212,8 @@
> };
> }};
>
> +// Only used by CBNZ, CBZ
> which is conditional based on
> +// a register value even though the
> instruction is always unconditional.
> def template
> BranchImmRegConstructor {{
> inline
> %(class_name)s::%(class_name)s(ExtMachInst machInst,
> int32_t _imm,
> @@
> -219,14 +221,7 @@
> : %(base_class)s("%(mnemonic)s", machInst,
> %(op_class)s, _imm, _op1)
> {
> %(constructor)s;
> - if (!(condCode ==
> COND_AL || condCode == COND_UC)) {
> - for (int x = 0; x < _numDestRegs;
> x++) {
> - _srcRegIdx[_numSrcRegs++] = _destRegIdx[x];
> - }
> -
> flags[IsCondControl] = true;
> - } else {
> - flags[IsUncondControl] =
> true;
> - }
> + flags[IsCondControl] = true;
> }
> }};
>
> Does that seem
> reasonable? Does it still improve performance on your
> test?
>
> Thanks,
>
> Ali
>
> On 08.03.2012 17:29, Brian Grayson wrote:
>
>> I
> think I may have found a bug in the handling for the cbz/cbnz ARM
> Thumb
>> instructions. They appear to set the IsUncondControl
> attribute/flag to
>> true, since this instruction does not have the
> typical condCode, but is
>> funneled through the common
> BranchImmRegConstructor(). This results in
>> badness in the branch
> predictor etc., especially during warm-up.
>> I have attached a patch
> that uses a distinct constructor for CBZ and CBNZ
>> that properly sets
> them as IsCondControl instructions. With this patch, the
>> branch
> prediction rate goes up by 0.5% for a few fast workloads I have lying
> around. I'm sure there are other ways of fixing this (like faking up a
> COND_EQ for cbz and COND_NE for cbnz that is then used by
> BranchImmRegConstructor), but this seemed the simplest way to get the
> job
>> done.
>>
>> Thanks.
>>
>> Brian Grayson
>>
>> diff --git
> a/src/arch/arm/isa/insts/branch.isa
> b/src/arch/arm/isa/insts/branch.isa
>> ---
> a/src/arch/arm/isa/insts/branch.isa
>> +++
> b/src/arch/arm/isa/insts/branch.isa
>> @@ -160,7 +160,7 @@
>>
>>
> {"code": code, "predicate_test": predTest},
>>
> ["IsIndirectControl"])
>> header_output +=
> BranchImmRegDeclare.subst(iop)
>> - decoder_output +=
> BranchImmRegConstructor.subst(iop)
>> + decoder_output +=
> BranchImmRegConstructor_CBZ_CBNZ.subst(iop)
>> exec_output +=
> PredOpExecute.subst(iop)
>> #TBB, TBH
>>
>> diff --git
> a/src/arch/arm/isa/templates/branch.isa
> b/src/arch/arm/isa/templates/branch.isa
>> ---
> a/src/arch/arm/isa/templates/branch.isa
>> +++
> b/src/arch/arm/isa/templates/branch.isa
>> @@ -230,6 +230,18 @@
>>
>>
> }
>> }};
>>
>> +def template BranchImmRegConstructor_CBZ_CBNZ {{
>>
>> +
> inline %(class_name)s::%(class_name)s(ExtMachInst machInst,
>> +
> int32_t _imm,
>> + IntRegIndex _op1)
>>
>> + :
> %(base_class)s("%(mnemonic)s", machInst, %(op_class)s, _imm,
>> _op1)
>>
>> + {
>>
>> + %(constructor)s;
>>
>> + // These are always conditional,
> but not in the usual predication
>> way.
>>
>> + flags[IsCondControl] =
> true;
>> + }
>>
>> +}};
>>
>> +
>>
>> def template BranchTarget {{
>>
>>
> ArmISA::PCState
>> _______________________________________________
>>
> gem5-dev mailing list
>> [email protected]
>>
> http://m5sim.org/mailman/listinfo/gem5-dev
>
>
> _______________________________________________
> gem5-dev mailing list
> [email protected]
> http://m5sim.org/mailman/listinfo/gem5-dev
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
