This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch 1/2][ARM]: New CPU support for Marvell Whitney
- From: Maxim Kuvyrkov <maxim dot kuvyrkov at linaro dot org>
- To: Xingxing Pan <xxingpan at marvell dot com>
- Cc: Kyrill Tkachov <kyrylo dot tkachov at arm dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Nick Clifton <nickc at redhat dot com>, Ramana Radhakrishnan <ramana dot radhakrishnan at arm dot com>, Richard Earnshaw <richard dot earnshaw at arm dot com>, James Greenhalgh <james dot greenhalgh at arm dot com>, Xinyu Qi <xyqi at marvell dot com>, Liping Gao <lgao1 at marvell dot com>
- Date: Thu, 26 Feb 2015 12:38:48 +0300
- Subject: Re: [patch 1/2][ARM]: New CPU support for Marvell Whitney
- Authentication-results: sourceware.org; auth=none
- References: <5492A8AD dot 1030203 at marvell dot com> <54940037 dot 6020205 at arm dot com> <54940573 dot 20307 at marvell dot com> <54AFBA04 dot 2040604 at arm dot com> <54B4DCCA dot 2040406 at marvell dot com> <54EDCEEC dot 7070302 at marvell dot com> <54EECCF2 dot 80400 at marvell dot com>
> On Feb 26, 2015, at 10:36 AM, Xingxing Pan <xxingpan@marvell.com> wrote:
>
> On 02/25/2015 09:32 PM, Xingxing Pan wrote:
>> Hi,
>>
>> This patch merges pipeline description for marvell-whitney to latest
>> code base.
>> Is it OK for trunk?
>>
> Refactor the commit message.
>
...
> Add pipeline description for marvell-whitney.
>
> 2015-02-26 Xingxing Pan <xxingpan@marvell.com>
>
> * config/arm/arm-cores.def: Add new core marvell-whitney.
> * config/arm/arm-protos.h:
> (marvell_whitney_vector_mode_qi): Declare.
> (marvell_whitney_inner_shift): Ditto.
> * config/arm/arm-tables.opt: Regenerated.
> * config/arm/arm-tune.md: Regenerated.
> * config/arm/arm.c (arm_marvell_whitney_tune): New structure.
> (arm_issue_rate): Add marvell_whitney.
> (marvell_whitney_vector_mode_qi): New function.
> (marvell_whitney_inner_shift): Ditto.
> * config/arm/arm.md: Include marvell-whitney.md.
> (generic_sched): Add marvell_whitney.
> (generic_vfp): Ditto.
> * config/arm/bpabi.h (BE8_LINK_SPEC): Add marvell-whitney.
> * config/arm/t-arm (MD_INCLUDES): Add marvell-whitney.md.
> * config/arm/marvell-whitney.md: New file.
> * doc/invoke.texi: Document marvell-whitney.
>
...
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 7bf5b4d..e68287f 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -2000,6 +2000,25 @@ const struct tune_params arm_cortex_a9_tune =
> ARM_SCHED_AUTOPREF_OFF /* Sched L2 autopref. */
> };
>
> +const struct tune_params arm_marvell_whitney_tune =
> +{
> + arm_9e_rtx_costs,
> + &cortexa9_extra_costs,
> + cortex_a9_sched_adjust_cost,
> + 1, /* Constant limit. */
> + 5, /* Max cond insns. */
> + ARM_PREFETCH_BENEFICIAL(4,32,32),
> + false, /* Prefer constant pool. */
> + arm_default_branch_cost,
> + false, /* Prefer LDRD/STRD. */
> + {true, true}, /* Prefer non short circuit. */
> + &arm_default_vec_cost, /* Vectorizer costs. */
> + false, /* Prefer Neon for 64-bits bitops. */
> + false, false, /* Prefer 32-bit encodings. */
> + false, /* Prefer Neon for stringops. */
> + 8 /* Maximum insns to inline memset. */
> +};
> +
You need to bootstrap GCC with your patch applied. As it is now, the above tune table is missing at least two entries (one for insn fusing, and one for sched L2 autoprefetcher). It should cause a bootstrap fail.
> const struct tune_params arm_cortex_a12_tune =
> {
> arm_9e_rtx_costs,
> @@ -11717,6 +11736,51 @@ fa726te_sched_adjust_cost (rtx_insn *insn, rtx link, rtx_insn *dep, int * cost)
> return true;
> }
>
> +/* Return true if vector element size is byte. */
> +bool
> +marvell_whitney_vector_mode_qi (rtx_insn *insn)
> +{
> + machine_mode mode;
> +
> + if (GET_CODE (PATTERN (insn)) == SET)
> + {
> + mode = GET_MODE (SET_DEST (PATTERN (insn)));
> + if (VECTOR_MODE_P (mode) && GET_MODE_INNER (mode) == QImode)
> + return true;
> + }
> +
> + return false;
> +}
It is preferable to avoid using predicates in DFA definitions. Predicates prevent optimizations of the DFA, which causes them to be bigger and slower. This predicate can be easily replaced by an attribute (set_attr "dest_vect_mode" "<mode>") on affected instructions (the proper name for the attribute needs some thinking though).
That said, I don't a have strong opinion on this case. It may be a worthwhile tradeoff to use the predicate to avoid adding new attribute to a dozen of instructions. If you / other reviewers prefer to keep the predicate -- please add a comment noting that it is used by the DFA.
> +
> +/* Return true if a non-shift insn has a shift operand. */
> +bool
> +marvell_whitney_inner_shift (rtx_insn *insn)
> +{
> + rtx pat = PATTERN (insn);
> +
> + if (GET_CODE (pat) != SET)
> + return false;
> +
> + /* Is not a shift insn. */
> + rtx rvalue = SET_SRC (pat);
> + RTX_CODE code = GET_CODE (rvalue);
> + if (code == ASHIFT || code == ASHIFTRT
> + || code == LSHIFTRT || code == ROTATERT)
> + return false;
> +
> + subrtx_iterator::array_type array;
> + FOR_EACH_SUBRTX (iter, array, rvalue, ALL)
> + {
> + /* Has shift operation. */
> + RTX_CODE code = GET_CODE (*iter);
> + if (code == ASHIFT || code == ASHIFTRT
> + || code == LSHIFTRT || code == ROTATERT)
> + return true;
> + }
> +
> + return false;
> +}
Same comment as above.
Otherwise looks good.
Thanks!
--
Maxim Kuvyrkov
www.linaro.org