[patch 1/2][ARM]: New CPU support for Marvell Whitney

[Maxim Kuvyrkov]

> 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