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
